Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not memoize auto/eager load paths in engines #49636

Merged
merged 1 commit into from Oct 14, 2023
Merged

Do not memoize auto/eager load paths in engines #49636

merged 1 commit into from Oct 14, 2023

Conversation

fxn
Copy link
Member

@fxn fxn commented Oct 14, 2023

Root issue

Rails 3 introduced engines and the way to configure them.

In particular, there are two ways to manipulate the auto/eager load paths:

  1. Defining paths with corresponding options, as in

    paths.add "app/models", eager_load: true
  2. Pushing custom paths to config.{autoload,autoload_once,eager_load}_paths).

For example, applications can do this:

config.paths["app/helpers"] << "#{Rails.root}/custom/helpers"
config.eager_load_paths << "#{Rails.root}/extras"

However, due to some internal memoizations, if you did that the other way around:

config.eager_load_paths << "#{Rails.root}/extras"
config.paths["app/helpers"] << "#{Rails.root}/custom/helpers"

then custom/helpers would not end up in the autoload and eager load paths. This is wrong.

Fix

To fix this, we let config.autoload_paths and friends do what the documentation says: These are collections where you put your custom stuff, period. Before, they would also collect and memoize what was in place at the time of the call via paths, which led to the bug explained above.

So, you can use both (1) and (2) now, and they won't interfere.

It's been +13 years, why now?

Newly generated Rails 7.1 applications by default push lib to config.eager_load_paths. If they are used together with something that edits config.paths afterwards, that latent bug now surfaces. I guess this combination, while possible, was not happening much in practice until now.

Extra ball

Since I was on it, the patch adds API documentation for them.

Fixes #49629.

@rails-bot rails-bot bot added the railties label Oct 14, 2023
@fxn
Copy link
Member Author

fxn commented Oct 14, 2023

I'll register this in the CHANGELOG in a dedicated commit, need to cherry-pick this one to main too.

@fxn fxn merged commit 02ed5a1 into 7-1-stable Oct 14, 2023
7 checks passed
@fxn fxn deleted the issue-49629 branch October 14, 2023 15:29
@chaadow
Copy link
Contributor

chaadow commented Oct 20, 2023

@fxn Hi, I'm running on the 7-1-stable and now rails zeitwerk:check reports all my app directories, as well as some gems ( as displayed in the screen)

Is there maybe an issue on my side on how i've configured the eager load paths?

I want these directories to be checked/eager loaded by rails zeitwerk:check to be sure everything is loaded properly

@fxn
Copy link
Member Author

fxn commented Oct 20, 2023

@chaadow the directories are indeed checked, but the warning is incorrect. I'll fix this.

@chaadow
Copy link
Contributor

chaadow commented Oct 20, 2023

@fxn Thanks!!

@chaadow
Copy link
Contributor

chaadow commented Oct 20, 2023

@fxn After some investigation, I've switched this line like this :

-  not_checked = ActiveSupport::Dependencies.autoload_paths - eager_load_paths
+  not_checked = Rails.configuration.autoload_paths - eager_load_paths
   not_checked.select! { |dir| Dir.exist?(dir) }
   not_checked.reject! { |dir| Dir.empty?(dir) }
}

And it made my warnings disappear. ( Although i'm not sure if this is the proper way to fix this)

If so, I can open a PR

@chaadow
Copy link
Contributor

chaadow commented Oct 20, 2023

Actually i think i figured it out, We have to use the new all_eager_load_paths method

-   eager_load_paths = Rails.configuration.eager_load_namespaces.filter_map do |eln|
-       # Quick regression fix for 6.0.3 to support namespaces that do not have
-      # eager load paths, like the recently added i18n. I'll rewrite this task.
-      eln.try(:config).try(:eager_load_paths)
-    end.flatten
+ eager_load_paths = Rails.configuration.eager_load_namespaces.filter_map do |eln|
+     # Quick regression fix for 6.0.3 to support namespaces that do not have
+    # eager load paths, like the recently added i18n. I'll rewrite this task.
+  eln.try(:config).try(:all_eager_load_paths)
+ end.flatten

}

@fxn
Copy link
Member Author

fxn commented Oct 20, 2023

Exactly :).

I'll leverage this to add test coverage for this task. This should have been prevented in CI.

@fxn fxn mentioned this pull request Oct 21, 2023
@fxn
Copy link
Member Author

fxn commented Oct 21, 2023

I have updated zeitwerk:check in #49739. Thanks for using 7-1-stable!

@mattbrictson
Copy link
Contributor

This PR seems to have caused turbo-rails to break when it is used without Action Cable. I logged an issue here with more details: hotwired/turbo-rails#512

$ bin/rails zeitwerk:check
Hold on, I am eager loading the application.
bin/rails aborted!
NameError: uninitialized constant ActionCable (NameError)

class Turbo::StreamsChannel < ActionCable::Channel::Base
                                                  ^^^^^^
Did you mean?  Rational
/Users/mbrictson/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/turbo-rails-1.5.0/app/channels/turbo/streams_channel.rb:34:in `<main>'

It's not clear to me whether the fix ultimately needs to be done in railties or in turbo-rails, but I thought I would add a comment in this PR for visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants