Rails Engine support

[tkowalewski]

I want to use view components in Rails Engine, but component generator produces component file without module (engine) name.
We can fix this manually always after component generation :|, but there should be support for module_namespacing like in templates in Rails.

How to reproduce

Create new Rails Engine

rails plugin new dummy --mountable

Add view_component as dependency in dummy.gemspec

spec.add_dependency "view_component", "~> 3.8"

and fix dummy.gemspec specs by adding homepage, summary ...

Install gems

bundle install

Require view component library in lib/dummy/engine.rb by adding at the top

require "view_component"

Generate view component

% bin/rails generate component Example
      create  app/components/dummy/example_component.rb
      invoke  test_unit
      create    test/components/dummy/example_component_test.rb
      invoke  erb
      create    app/components/dummy/example_component.html.erb

And generated component (app/components/dummy/example_component.rb) looks like

# frozen_string_literal: true

class ExampleComponent < ViewComponent::Base
  def initialize(text:)
    @text = text
  end

end

Expected behaviour

ViewComponent should generate components within Rails Engine module like all other resources generated for Rails Engine.

Expected component

# frozen_string_literal: true

module Dummy
  class ExampleComponent < ViewComponent::Base
    def initialize(text:)
      @text = text
    end
  end
end

The fix

To generate component within Rails Engine module we should use module_namespacing method in templates.
For example lib/rails/generators/component/templates/component.rb.tt should looks like:

# frozen_string_literal: true

<% module_namespacing do -%>
class <%= class_name %>Component < <%= parent_class %>
<%- if initialize_signature -%>
  def initialize(<%= initialize_signature %>)
    <%= initialize_body %>
  end
<%- end -%>
<%- if initialize_call_method_for_inline? -%>
  def call
    content_tag :h1, "Hello world!"<%= ", data: { controller: \"#{stimulus_controller}\" }" if options["stimulus"] %>
  end
<%- end -%>
end
<% end -%>

[reeganviljoen]

@joelhawksley @camertron @Spone @boardfish any thought's on this

[boardfish]

Sounds to me like the solution is well understood here, so we can go ahead with the described change. @tkowalewski, would you like to open a pull request for this?

[tkowalewski]

@boardfish Yep, I will prepare pull request soon. Thanks

[tkowalewski]

Work in progress (#1951)

[Spone]

You can leave the issue open while working on the PR.

[reeganviljoen]

@tkowalewski is their any reason you closed this, because it may still be useful to track the progress of this feature(normally we close the issue once the PR has been merged by adding a closses <issue_num> to the PR description)

[tkowalewski]

Oh, ok

I thought it would be better to have conversations in one place (in the PR)

[jfi]

Thanks for your work on this @tkowalewski – trying to get this working at the moment (but with spec rather than test, if you could bear that in mind) – managed to get the components loading by changing the paths, but struggling with preview paths from within the engine at the moment! Watching with interest... 👀

[Slotos]

There's another issue with view_component when it comes to isolated engines - it uses a global configuration. This means that a dummy app can configure config.view_component in one way, and have it tested, but when the engine gets mounted, config.view_component can differ and break expectations.

Right now this is clearly visible with render monkeypatch, preview settings, test controller settings, and url helpers included in view components by default:

• Disabling render monkeypatch can break engines outright

  Engines sticking to render_component avoids the issue.

• Preview doesn't necessarily make sense in the main app, e.g. if it doesn't use view_component

  There's probably a setup where this is outright breaking, but without one it can be postponed for now.

• Global test controller setting enforces use of with_controller_class in engine component tests

  Allow specifying this on a class level that propagates through the inheritance tree.

  E.g.:

  def vc_test_controller
    @vc_test_controller ||= __vc_test_helpers_build_controller(test_controller || Base.test_controller.constantize)
  end

• ViewComponent::Base includes rails application url helpers, forcing use of helpers.something_path, whereas views that are being componentised stick to engine url helpers

  ViewComponent::EngineBase that skips the following could partially deal with the problem:

  # If Rails application is loaded, add application url_helpers to the component context
  # we need to check this to use this gem as a dependency
  if defined?(Rails) && Rails.application && !(child < Rails.application.routes.url_helpers)
    child.include Rails.application.routes.url_helpers
  end

  Subclasses can then include EngineName.instance.routes.url_helpers to have components behave similar to views that are being componentised.

[joelhawksley]

There's another issue with view_component when it comes to isolated engines - it uses a global configuration. This means that a dummy app can configure config.view_component in one way, and have it tested, but when the engine gets mounted, config.view_component can differ and break expectations.

Sound familiar @reeganviljoen? (#1987)

It sounds like we need to consider moving away from global ViewComponent configuration. @boardfish I know you've done a lot of work on our config architecture, what do you think about all of this?

[reeganviljoen]

@joelhawksley I am just on holiday now I can take a better look at this next week and let you know.

[reeganviljoen]

@joelhawksley I think this is a good idea, but this means we need to make config local to something, so do we make it local to a component, an engine, or anything else?

[joelhawksley]

@reeganviljoen I'm thinking we make it per-component, with the expectation that folks would set config on ApplicationComponent.

[tkowalewski]

Thanks for your work on this @tkowalewski – trying to get this working at the moment (but with spec rather than test, if you could bear that in mind) – managed to get the components loading by changing the paths, but struggling with preview paths from within the engine at the moment! Watching with interest... 👀

@jfi In this pull request (#1951) I am trying to focus on support for generating components in rails engine.
Rails engine is generated with tests by default (you can use flag to disable).
I see issue with support for rspec but for now I don't have time to investigate it more. I will try fix all rspec problems in another pull request.

[boardfish]

@joelhawksley Apologies, this thread fell by the wayside! I'd like to take a deeper look into this – off the top of my head, the first thing that comes to mind is how Rails engines get generated with their own internal ApplicationController, ApplicationRecord etc.. This makes me wonder if ViewComponent::Base should have a static default configuration, and all current means of configuring ViewComponent should instead be directed towards a config object that lives on ApplicationComponent?

I'd want to make sure that that's also compatible with engines, but it's harder to make assumptions about where ApplicationComponent is going to live in those.

[boardfish]

I've been mulling over what this looks like, because I'm expecting that having a config object per component won't be performant. I think what we should do is have an underlying map of object names to config instances, e.g. (pseudocode):

{
  "ApplicationComponent" => config instance,
  "SomeComponent" => another ViewComponent::Config instance
}

Then, when ViewComponent::Base.config is called:

1. we pass off to a delegator object that checks to see if config already exists for this class in the hash

If we're writing to the config:

1. create the config for this class if it doesn't exist
2. write to the config

If we're reading from the config:

1. if it doesn't exist, go up the ancestral chain and check for the first class that has a config
2. read from the first config found

We could make this backwards compatible with the current config setup by making it (optionally) possible to write to the config for ViewComponent::Base as we do now and plan to deprecate that soon. All current ways of writing to the config would do so for ApplicationComponent instead and we could encourage folks to inherit from that by default.

[joelhawksley]

I'm expecting that having a config object per component won't be performant

What if we stored the config as class variables at load time and used the inheritance hierarchy builtin to Ruby?

[reeganviljoen]

@joelhawksley @boardfish I have two major concerns with a component based config system as presented above:

• How would config that needs to be global i.e preview_path work
• The current system is more consistent with how Rails creates configs so that is more approachable and fits with the rails convention

[joelhawksley]

@reeganviljoen Good point. I think we'd need engine-level config for some things for sure.

[boardfish]

@joelhawksley I was a little averse to this at first, because I think it'd be more difficult to do cleanly – I can't visualise it quite as easily. But it's definitely possible using, say, class_attributes that we include onto component classes. I like the current structure of having the config defined as an object, but that's definitely in the balance as far as making sure this is still performant.

@reeganviljoen I'm with you there – we'd definitely need to have some separation of what's global and what's not. The line there really comes down to what happens during app initialization, for the most part – things like the capture compatibility patch and preview paths might not be able to be toggled at runtime.

[boardfish]

I've had a look at doing this just now. The thing that makes this difficult is load order – I get the feeling there will need to be separation of app-level config that's checked at app load time and component-level config that we check once ViewComponent::Base has been initialized. The way both of these things overlap with engines also remains to be figured out.

The thing that necessitated Config objects that are stored separately in the first instance was load order, but I think @joelhawksley's suggested approach of potentially using class attributes makes a lot of sense on a per-component basis.

I wanted to avoid this being a breaking change, but I think that's practically impossible if we're using class attributes on component classes, as we won't be able to set those during engine initialization because they won't have loaded yet. They bring Action View with them, so we also couldn't if we tried.

TL;DR: it's a big lift and a breaking change, but it's not impossible! I'll keep on with it.