Avoid loading ActionView::Base during initialization

[jdelStrother]

What are you trying to accomplish?

ActionView::Base shouldn't be loaded during the initialization process.
Rails does a poor job of making this explicit, IMO - there's a suggestion that you shouldn't load frameworks (ActiveRecord::Base, ActionView::Base etc) during boot here: https://edgeguides.rubyonrails.org/engines.html#load-and-configuration-hooks, since it slows the boot process.
More concretely, loading those frameworks too soon can mean that they ignore configuration - eg setting config.active_record.belongs_to_required_by_default in config/initializers/new_framework_defaults.rb is ignored if ActiveRecord::Base has already been loaded.
As it happens, ActionView::Base doesn't have the same problem because most of its settings are propagated to, eg, ActionView::Helpers::TagHelper in after_initialize, rather than being set directly on ActionView:Base, but that seems a fragile thing to rely on.

#1507

What approach did you choose and why?

I've started by making the test suite fail if Rails frameworks load too soon here: 50f0bf0 ... it's not great, I'm open to better suggestions. It's not an individual test, it just nukes the entire test process.

f10920a then fixes this by making the VC::Engine (rather than VC::Base) responsible for loading VC::Config, and the engine sets that config on VC::Base after initialization is complete (in ActiveSupport.on_load(:view_component) { ... })

I think it might be more rails-y to have something like this in the initializer:

modified   lib/view_component/engine.rb
@@ -36,7 +36,9 @@ module ViewComponent
 
     initializer "view_component.enable_instrumentation" do |app|
       ActiveSupport.on_load(:view_component) do
-        Base.config = app.config.view_component
+        app.config.view_component.each do |k, v|
+          send("#{k}=", v)
+        end
 
         if app.config.view_component.instrumentation_enabled.present?
           # :nocov:

which I mostly had working, although this integration test fails because it seems to expect changing Base.config to affect app.config.view_component and vice versa.
I'm not sure I agree with that (eg setting ActiveRecord::Base.verbose_query_logs = true doesn't change config.active_record.verbose_query_logs), but for the sake of not breaking backwards compatibility have left it as-is. I don't have a strong opinion either way.

Anything you want to highlight for special attention from reviewers?

I'm not quite sure what the original code was doing here -

view_component/lib/view_component/engine.rb

Lines 17 to 19 in 88cec82

	%i[generate preview_controller preview_route show_previews_source].each do |config_option|
	options[config_option] ||= ViewComponent::Base.public_send(config_option)
	end

. options on line 18 is the exact same object as VC::Base.config, so it seems like that boils down to

  options = ViewComponent::Base.config
  options[:generate] ||= options.send(:generate)
  options[:preview_controller] ||= options.send(:preview_controller)
  options[:preview_route] ||= options.send(:preview_route)
  options[:show_previews_source] ||= options.send(:show_previews_source)

but let me know if I'm missing something dumb.

[jdelStrother]

Bah, will try & take a look at the pvc failure later

[jdelStrother]

I've added require "view_component/config" to the top of engine.rb so that it still works for people requiring view_component/engine directly. I think that could be removed in 3.0 in favour of autoloads.

[joelhawksley]

👏🏻 love that test. Thank you!