Closed tkowalewski closed 1 month ago
@joelhawksley @camertron @Spone @boardfish any thought's on this
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?
@boardfish Yep, I will prepare pull request soon. Thanks
Work in progress (https://github.com/ViewComponent/view_component/pull/1951)
You can leave the issue open while working on the PR.
@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
Oh, ok
I thought it would be better to have conversations in one place (in the PR)
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... :eyes:
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.
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? (https://github.com/ViewComponent/view_component/pull/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?
@joelhawksley I am just on holiday now I can take a better look at this next week and let you know.
@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?
@reeganviljoen I'm thinking we make it per-component, with the expectation that folks would set config on ApplicationComponent.
Thanks for your work on this @tkowalewski – trying to get this working at the moment (but with
spec
rather thantest
, 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 (https://github.com/ViewComponent/view_component/pull/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.
@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.
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:
If we're writing to the config:
If we're reading from the config:
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.
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?
@joelhawksley @boardfish I have two major concerns with a component based config system as presented above:
preview_path
work@reeganviljoen Good point. I think we'd need engine-level config for some things for sure.
@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_attribute
s 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.
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.
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
Add
view_component
as dependency indummy.gemspec
and fix
dummy.gemspec
specs by addinghomepage
,summary
...Install gems
Require view component library in
lib/dummy/engine.rb
by adding at the topGenerate view component
And generated component (
app/components/dummy/example_component.rb
) looks likeExpected behaviour
ViewComponent should generate components within Rails Engine module like all other resources generated for Rails Engine.
Expected component
The fix
To generate component within Rails Engine module we should use
module_namespacing
method in templates. For examplelib/rails/generators/component/templates/component.rb.tt
should looks like: