Fix helpers not reloading in previews

[jdelStrother]

Summary

Helpers don't seem to get reloaded when working with component previews (#1069).

This PR adds 2 tests.
One demonstrating that helpers are successfully reloaded when components are rendered through a 'normal' controller.
The other attempts to do the same via ViewComponentsController#previews, but the test fails since the helper isn't reloaded.

Unfortunately I had to set config.cache_classes=false to get this working, which causes a couple of other test failures. I'm not sure there's a good way to only set that for the two tests I added, unless we want to spawn a brand new test process for them.

[joelhawksley]

@jdelStrother thank you for providing test cases 🙏🏻

@juanmanuelramallo would you be up for having a look into this?

[juanmanuelramallo]

Hey @jdelStrother thanks for the failing tests!

Please have a look at this commit and let me know what you think. This commit solves the reloading issue with helpers. (cc @joelhawksley)

We'd still need to solve the other tests that fail due to the cache_classes configuration.

[camertron]

I spent some time yesterday looking into this as well and came to the same conclusion. This line fixed the failing test. However, it made another test fail. Do you know why the inheritance matters here?

[juanmanuelramallo]

The other failures in this branch seem to be related to the cache_classes configuration change. I wasn't able to dynamically update that config at runtime and make it work as expected.

I think the actual issue is that rails/application_controller is not reloaded by zeitwerk, perhaps because we require it manually and/or we don't have it present in the autoload_paths. BTW I just noticed we should remove the require line as well.

[jdelStrother]

Hey @jdelStrother thanks for the failing tests!

Please have a look at this commit and let me know what you think. This commit solves the reloading issue with helpers. (cc @joelhawksley)

Thanks - looks promising. I'm away for a week though, will try it on my return

[jdelStrother]

Confirming this does fix the reloading issues for me. However, I found a possible alternate fix...

It looks like Rails only re-includes reloaded helpers in direct subclasses of ActionController::Base (https://github.com/rails/rails/blob/f1f86603a96b965e28839e418a5712325bf9ca38/actionpack/lib/action_controller/railties/helpers.rb#L18), which is why @juanmanuelramallo's fix works.

So, ViewComponentsController could do something like this:

diff --git a/app/controllers/view_components_controller.rb b/app/controllers/view_components_controller.rb
index fbc6e32..e3e1452 100644
--- a/app/controllers/view_components_controller.rb
+++ b/app/controllers/view_components_controller.rb
@@ -8,6 +8,7 @@ class ViewComponentsController < Rails::ApplicationController # :nodoc:
   around_action :set_locale, only: :previews
   before_action :find_preview, only: :previews
   before_action :require_local!, unless: :show_previews?
+  helper :all

   if respond_to?(:content_security_policy)
     content_security_policy(false)

which ensures that when ViewComponentsController is reloaded, it re-includes the app's helper modules. Of course, that assumes that we want to include :all helper modules... have you heard from any users wanting to only include specific helpers in ViewComponentsController ?

[juanmanuelramallo]

I like your approach better. Less changes and way simpler.

Do you mind reverting my commits in favor of the change you proposed?

[jdelStrother]

Couple of thoughts -

• By adding helper :all to ViewComponentsController, we'll be including the helpers twice - a non-reloadable version in Rails::ApplicationController, and a reloadable version in ViewComponentsController. Not ideal, but I think it's probably fine in practice.
• What do you want to do about the tests? I think the options are:
  • Leave reloading helpers untested
  • Disable cache_classes for the entire test suite, and see if we can fix the failing tests that that has produced
  • Make the test suite much more complicated and have it fork off entire new rails tests processes with different settings in test.rb

[joelhawksley]

@jdelStrother would you be interested in picking this PR back up?

[jdelStrother]

Sure. Any thoughts on my latest comment?

[joelhawksley]

Leave reloading helpers untested

I think we'd want to avoid that situation 😄

How about we start with turning class caching off and see how that goes?

[jdelStrother]

Not sure if it's worth keeping this. UnreferencedComponent will get compiled on boot (because eager_load=true) but it might get reloaded before this test runs (because cache_classes=false).

I assume it's trying to test this line -

view_component/lib/view_component/engine.rb

Line 68 in 964a2c1

ViewComponent::Base.descendants.each(&:compile) if Rails.application.config.eager_load

If we reeaaally want to test that unreferenced components get compiled on boot, we could start a new rails process within the test:

  def test_compiles_unrendered_component
    assert_equal "true\n", `ruby -r bundler/setup -r ./test/sandbox/config/environment.rb -e "puts UnreferencedComponent.compiled?"`
  end

[camertron]

Since it's unreferenced, why would it get reloaded?

[jdelStrother]

It'll get eager-loaded during boot due to

view_component/test/sandbox/config/environments/test.rb

Line 45 in d6b385e

config.eager_load = true

and then reloaded whenever we call app.reloader.reload!

[camertron]

How about this?

diff --git a/test/sandbox/config/environments/test.rb b/test/sandbox/config/environments/test.rb
index 4ec120f9..18bf3fd3 100644
--- a/test/sandbox/config/environments/test.rb
+++ b/test/sandbox/config/environments/test.rb
@@ -45,4 +45,40 @@ Sandbox::Application.configure do
   config.action_view.annotate_rendered_view_with_filenames = true if Rails.version.to_f >= 6.1
 
   config.eager_load = true
+
+  # Components that should be ignored when `app.reloader.reload!` is called in tests.
+  config.components_to_ignore_when_reloading = [
+    "UnreferencedComponent"
+  ]
+
+  config.before_eager_load do
+    config.components_to_ignore_when_reloading.each do |const_name|
+      # Make sure the constant is loaded so we can get its source location
+      Kernel.const_get(const_name)
+      path, _ = Kernel.const_source_location(const_name)
+
+      # Instruct Zeitwerk to ignore this file (only works the second time around)
+      Rails.application.autoloaders.main.ignore(path)
+
+      # Manually remove the constant from the list of constants to unload. Unloading
+      # is done on setup and reload.
+      Rails.application.autoloaders.main.send(:to_unload).delete(const_name)
+    end
+  end
+
+  # The compile cache is cleared whenever `app.reloaders.reload!` is called. This monkeypatch
+  # ensures ignored components are skipped.
+  ViewComponent::CompileCache.singleton_class.prepend(
+    Module.new do
+      def invalidate!
+        ignore_list = Rails.application.config.components_to_ignore_when_reloading
+
+        cache.each do |klass|
+          next if ignore_list.include?(klass.name)
+
+          invalidate_class!(klass)
+        end
+      end
+    end
+  )
 end
diff --git a/test/sandbox/test/rendering_test.rb b/test/sandbox/test/rendering_test.rb
index e4dbdd99..276cc01a 100644
--- a/test/sandbox/test/rendering_test.rb
+++ b/test/sandbox/test/rendering_test.rb
@@ -416,11 +416,8 @@ class RenderingTest < ViewComponent::TestCase
     assert_includes exception.message, "Validation failed: Content"
   end
 
-  def test_compiles_unrendered_component
-    skip "this might not still be compiled if it's been reloaded since boot"
+  def test_compiles_unreferenced_component
     assert UnreferencedComponent.compiled?
-    # Or a possible alternative is booting a fresh process:
-    # assert_equal "true\n", `ruby -r bundler/setup -r ./test/sandbox/config/environment.rb -e "puts UnreferencedComponent.compiled?"`
   end
 
   def test_compiles_components_without_initializers

[jdelStrother]

OK, I've rebased & fixed up the test failures with cache_classes=false. (Well, mostly. One of them I've marked with skip - see comments)

[camertron]

Hey @jdelStrother I took some time to go through this in detail today and I think it's almost ready. I was going to take a crack at the test failures but there were some conflicts merging main and I wasn't confident I could do it without making a mistake. Could you merge main?

[jdelStrother]

OK, updated

[camertron]

Thanks @jdelStrother! Looks like we had some flaky test failures, so I re-ran and got an almost clean build. Something's going on with code coverage tho. At least one of the bits of uncovered code was committed 2 years ago, so I have a feeling the flaky test failures are interfering with the coverage numbers. Hopefully that'll be resolved on the next commit and push.

[jdelStrother]

Something's going on with code coverage tho

Mm. I notice that spec_helper & test_helper use the same command_name - won't that result in them overwriting each others' coverage results?

Sadly, that's not the real issue. I think the real issue is that simplecov doesn't support coverage of reloaded files - simplecov-ruby/simplecov#389 - and preview_helper.rb is getting reloaded. I'm struggling to figure out a good way around it.

[camertron]

I notice that spec_helper & test_helper use the same command_name - won't that result in them overwriting each others' coverage results?

Huh, yes it will, although it doesn't seem to be? Not sure what's going on here.

I think the real issue is that simplecov doesn't support coverage of reloaded files

You're absolutely right. I messed around with this for a long time today and wasn't able to get anywhere. I tried dumping the coverage results before reloading, but for some reason that doesn't work (it identifies a bunch of uncovered lines that are definitely covered).

As mentioned in the issue you linked, this appears to be a problem in Ruby's coverage module. Or rather, it's expected behavior we can't do anything about. Whatever we would do is likely to be pretty hacky too 😢

What if we ran the test suite multiple times, once for coverage and once to test reloading behavior? We could skip the test you added if config.cache_classes is true.

[jdelStrother]

I notice that spec_helper & test_helper use the same command_name - won't that result in them overwriting each others' coverage results?

Huh, yes it will, although it doesn't seem to be? Not sure what's going on here.

It's only setting the command name if ENV["RUBY_VERSION"] is set, which it never is AFAICT. So simplecov by default is naming them "RSpec" and "Unit test" - see #1750

Not sure what's up with the failing tests, will try & take a closer look later. Bad MiniTest version ??

OK, think they're all fixed.

[camertron]

Awesome! Thanks for seeing this all the way through 🎉