Load paths hash #190
Load paths hash #190
Conversation
| end | ||
| Rails.autoloaders.inject({}) do |h, loader| | ||
| h.merge(loader.root_dirs) | ||
| end |
There was a problem hiding this comment.
So it turns out this doesn't work in Rails 6.0 unless Rails.autoloaders.zeitwerk_enabled? returns true. If it is false, Rails.autoloaders will be an empty enumerable.
So now we have to choose one of:
- For all the places that we stub out
config.autoload_pathsand friends we add matching tests to cover when zeitwerk is both enabled and disabled. - Explicitly add the requirement that packwerk is only compatible with Rails applications which are zeitwerk enabled. This is the case for Rails 6.x codebases that have
Rails.autoloaders.zeitwerk_enabled? == trueor Rails 7.0+ codebases.
Personally, based on our discussions about adding Zeitwerk as a dev dependency in the test in this diff I feel we could pretty safely say that Packwerk requires zeitwerk_enabled? to be true. This would retain Rails 6.x support without requiring us to duplicate a whole bunch of test.
As "Classic mode" isn't coming back, and Packwerk is based on the conventions of Zeitwerk, unless this causes problems for any known users, I'd suggest we go with the addition of this "soft" requirement.
Edit: I've edited the rails_test_helper Dummy application to explicitly have config.autoloader = :zeitwerk for now to get tests passing.
There was a problem hiding this comment.
Maybe we should make it more obvious, but packwerk already officially requires zeitwerk to be enabled https://github.com/Shopify/packwerk#prerequisites
If you put up a PR to make it more obvious I'll accept it
|
Anyone with more Sorbet knowledge than I know what to do with this error? |
Looks like we don't have a type spec for |
I couldn't really get it working, probably my lack of experience with |
|
This should be a non-breaking change right? I'll try to find some time to run it against our monolith. |
|
our main monolith CI is green against this branch. I've also done some manual testing and it's looking great. |
|
@mclark do you think it'd make ense to reduce the number of commits (squash some together) before we merge? |
This changes a couple things when running Packwerk against a Rails app. First of all, instead of getting our load paths via `config.autoload_paths` and related methods, we use Rails' zeitwerk autoloaders. Secondly, instead of representing the load paths as a list of root directory paths, we instead use a hashmap of root directory paths to module instances which are the root modules for the path. This allows users to override the default module for any root directory.
What are you trying to accomplish?
As described in #186, Zeitwerk supports the notion of a "default" namespace for each root directory. This is represented by a
Hashof root directory path strings to constants representing the default module inZeitwerk::Loader#root_dirs. This pull request updates Packwerk so that it treats the group of load paths as a hash rather than an Array and uses the#root_dirsof each autoloader inRails.autoloadersinstead.What approach did you choose and why?
As Packwerk relies on Rails'
config.autoload_pathsand friends lists to get the list of load paths to draw on for its constant discovery process, and these are just lists of strings, they needed to be abandoned in favour of relying on the autoloaders instead which are hashes of root dir paths to default modules. This triggered a number of changes, mostly to method signatures as the new hash worked its way through the call stacks to eventually arrive in theConstantResolvergem (already updated in Shopify/constant_resolver#27).After that, most of the effort just went into updating the tests to conform to the new expectations:
RailsPathsin favour of using plain old Mocha mocksRails.autoloaderswould be populatedActiveSupport::TestCasetoMinitest::Testto be consistent with all other tests and to suppress log output caused by initializing the dummy application.What should reviewers focus on?
I believe @exterm is bumping
constant_resolveras I write this so expect the git dependency forconstant_resolverto go away and be replaced once that is done.I am not an expert on rails or load paths. Is eager loading still supported properly? Can we suppress the logging in a better way than outputting "nothing" to standard out?
Type of Change
Additional Release Notes
Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.
If no additional notes are necessary, delete this section or leave it unchanged.
Checklist