Improve eager loading Rails engines when Zeitwerk is present #1329
Conversation
egiurleo
force-pushed
the
emily/properly-load-rails-engines
branch
from
05a9bef
to
965c3ca
Compare
paracycle
force-pushed
the
emily/properly-load-rails-engines
branch
from
fa18dfe
to
2d93c45
Compare
egiurleo
force-pushed
the
emily/properly-load-rails-engines
branch
from
715a2c4
to
97aec17
Compare
egiurleo
force-pushed
the
emily/properly-load-rails-engines
branch
3 times, most recently
from
988dd2e
to
7f512ac
Compare
egiurleo
force-pushed
the
emily/properly-load-rails-engines
branch
2 times, most recently
from
4588d91
to
401b188
Compare
There was a problem hiding this comment.
🎉
I think I'll close Shopify/rbi-central#135 since Zeitwerk is default for Rails 6+
| def rails_engines | ||
| return [] unless Object.const_defined?("Rails::Engine") | ||
|
|
||
| safe_require("active_support/core_ext/class/subclasses") | ||
|
|
||
| project_path = Bundler.default_gemfile.parent.expand_path | ||
| # We can use `Class#descendants` here, since we know Rails is loaded | ||
| Object.const_get("Rails::Engine") |
There was a problem hiding this comment.
I'm curious, was there a reason these used to be const_get?
There was a problem hiding this comment.
That is a really good question and I'm not sure why, besides maybe it needed to be that way at some point and nobody ever fixed it? I'm assuming that because we already had the Object.const_defined?("Rails::Engine") check in place, this change could have been made earlier. @paracycle might know!
There was a problem hiding this comment.
I think it was a combination of me being less experienced when I originally wrote this code about 3.5 years ago, and, also, the fact that Tapioca did not have a dev-time Rails dependency, thus, Sorbet didn't know about constants like Rails and would raise static type-checking errors.
lib/tapioca/loaders/loader.rb
Outdated
| rescue NameError | ||
| nil |
There was a problem hiding this comment.
I don't think we can still get a NameError in this method, due to the defined? check. So we can remove this.
lib/tapioca/loaders/loader.rb
Outdated
| Rails.app_class = Rails.application = rails_application | ||
| end | ||
|
|
||
| T::Sig::WithoutRuntime.sig { returns(T::Array[T.class_of(Rails::Engine)]) } | ||
| def rails_engines | ||
| return [] unless Object.const_defined?("Rails::Engine") |
There was a problem hiding this comment.
I think we can make this:
| return [] unless Object.const_defined?("Rails::Engine") | |
| return [] unless defined?(Rails::Engine) |
which reads better than const_defined? and the constant reference can be typechecked.
RBI generation for Rails engines is currently implemented with a best effort approach. We identify every file in the application's eager_load_path and attempt to require those files. This does not account for dependency order, so we keep track of any files that fail in the process and then try to require them again once all the other files are loaded. This approach is not exhaustive and can sometimes miss constants defined in engines. For example, if an engine defines a constant (e.g. Turbo::Streams) as a namespace for subconstants, but never on its own, the current approach will never generate type annotations for that constant. This implementation improves engine eager loading when Zeitwerk is present in the Rails application. Rather than relying on the best effort approach, we use the behavior already implemented in Zeitwerk to eager load every file in the application's eager_load_path, which is much more reliable. The resulting RBI will contain constants defined in the engine that wouldn't have been found by the previous approach. If Zeitwerk is not present, then we fall back to the best effort approach that we were already using.
…t are only ever defined as namespaces
egiurleo
force-pushed
the
emily/properly-load-rails-engines
branch
from
96d3953
to
b1beb6c
Compare
|
Okay I think I've addressed everyone's comments. I also squashed all the little commits into one big one and gave it a good description. |
Motivation
Closes #671
Closes Shopify/rbi-central#135
RBI generation for Rails engines is currently implemented with a best effort approach. We identify every file in the application's
eager_load_pathand attempt to require those files. This does not account for dependency order, so we keep track of any files that fail in the process and then try to require them again once all the other files are loaded.This approach is not exhaustive and can sometimes miss constants defined in engines. For example, if an engine defines a constant (e.g.
Turbo::Streams) as a namespace for subconstants, but never on its own, the current approach will never generate type annotations for that constant.Implementation
This implementation improves engine eager loading when Zeitwerk is present in the Rails application. Rather than relying on the best effort approach, we use the behavior already implemented in Zeitwerk to eager load every file in the application's
eager_load_path, which is much more reliable. The resulting RBI will contain constants defined in the engine that wouldn't have been found by the previous approach.If Zeitwerk is not present, then we fall back to the best effort approach that we were already using.
Tests
We have added a test that passes with these changes and fails without them.