Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Failing tests demonstrating helpers not reloading in previews #1075
Conversation
|
@jdelStrother thank you for providing test cases 🙏🏻 @juanmanuelramallo would you be up for having a look into this? |
Rails::ApplicationController was being manually required and therefore not properly reloaded by Zeitwerk. This commit removes the dependency with Rails::ApplicationController by using ActionController::Base instead. It copies over some before_action methods from https://github.com/rails/rails/blob/main/railties/lib/rails/application_controller.rb so that the behavior is kept the same. Finally it also includes a base layout copied from https://github.com/rails/rails/blob/main/railties/lib/rails/templates/layouts/application.html.erb
|
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. |
| @@ -2,12 +2,14 @@ | |||
|
|
|||
| require "rails/application_controller" | |||
|
|
|||
| class ViewComponentsController < Rails::ApplicationController # :nodoc: | |||
| class ViewComponentsController < ActionController::Base # :nodoc: | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks - looks promising. I'm away for a week though, will try it on my return |
|
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 |
|
I like your approach better. Less changes and way simpler. Do you mind reverting my commits in favor of the change you proposed? |
|
Couple of thoughts -
|
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=falseto 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.