-
Notifications
You must be signed in to change notification settings - Fork 121
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
Fix engine loading for Rails < 7.2 #1707
Conversation
d934b0c
to
9347ab3
Compare
lib/tapioca/loaders/loader.rb
Outdated
# https://github.com/rails/rails/commit/ebfca905db14020589c22e6937382e6f8f687664 | ||
eager_load_paths = if Rails::VERSION::MAJOR >= 7 && Rails::VERSION::MINOR >= 2 | ||
engine.config.all_eager_load_paths | ||
else | ||
engine.config.eager_load_paths | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a huge fan of version checks if we don't have to, feature detection is always nicer.
Can we instead check for the existence of the all_eager_load_paths
method instead and encapsulate the lookup in a method?
# https://github.com/rails/rails/commit/ebfca905db14020589c22e6937382e6f8f687664 | |
eager_load_paths = if Rails::VERSION::MAJOR >= 7 && Rails::VERSION::MINOR >= 2 | |
engine.config.all_eager_load_paths | |
else | |
engine.config.eager_load_paths | |
end | |
eager_load_paths = eager_load_paths(engine) |
and
def eager_load_paths(engine)
config = engine.config
(config.respond_to?(:all_eager_load_paths) && config.all_eager_load_paths) || config.eager_load_paths
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I made the change.
However, we still need to duplicate the logic for the symbol loader, which I don't exactly love.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's not great, but I think the real fix is to extract an engine handler class to unify the logic. Not really a concern that needs to be addressed by this PR, though.
9347ab3
to
e42529f
Compare
I also upgraded |
Note: the latest RBI has the fixes we need and I opened a PR to upgrade our dependency #1708, which I can merge immediately after this one. |
I don't quite get why this would be the case. Can you explain more? |
We added automatic RBI generation to Prism, which means Tapioca picks it up from That unfortunately means that CI would never pass. If we generated the RBIs for Version v0.17.1 fixes the RBIs, so now we can properly generate it. |
Motivation
Closes #1706
In #1702, I was trying to avoid having to spread a bunch of conditionals in our code to handle the Rails 7.2 configuration rename. For some reason, the approach doesn't work properly for Rails < 7.2.
Implementation
Switched the configuration patching to just use conditionals where we look for eager load paths.
Tests
We have a specific test for verifying that RBIs include classes defined under the
app
folder in engines. I'm not sure why this isn't failing in the Rails main build.I verified that this works on Rails 7.2 and older manually.