Use Zeitwerk to autoload files #5425
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javierm
force-pushed
the
zeitwerk
branch
3 times, most recently
from
2251e50
to
b7e1d19
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
javierm
force-pushed
the
zeitwerk
branch
2 times, most recently
from
5e34d23
to
3234517
Compare
Merged
javierm
force-pushed
the
zeitwerk
branch
3 times, most recently
from
75ded42
to
def6a0c
Compare
We use vendor/assets and app/assets; the purpose of lib/assets isn't that clear, though. According to the Rails guides: > lib/assets is for your own libraries' code that doesn't really fit > into the scope of the application or those libraries which are shared > across applications. So it must be something for companies having several Rails applications, which isn't our case. Furthermore, this text has been removed from the Rails guides for version 7.1, so this folder might be a legacy folder.
This monkey-patch doesn't seem to be working with Zeitwerk, and we were only using it in one place, so the easiest way to solve the problem is to remove it. Note that, in the process, we're changing the operation so `* 100` appears before the division, so it's consistent with other places where we do similar things (like the `supports_percentage` method in the proposals helper).
Since we autoload the `lib` folder, there's no need to manually require the files inside it.
The purpose of the lib folder is to have code that doesn't necessary belong in the application but can be shared with other applications. However, we don't have other applications and, if we did, the way to share code between them would be using a gem or even a git submodule. So having both the `app/` and the `lib/` folders is confusing IMHO, and it causes unnecessary problems with autoloading. So we're moving the `lib/` folder to `app/lib/`. Originally, some of these files were in the `app/services/` folder and then they were moved to the `lib/` folder. We're using `app/lib/` instead of `app/services/` so the upgrade is less confusing. There's an exception, though. The `OmniAuth::Strategies::Wordpress` class needs to be available in the Devise initializer. Since this is an initializer and trying to autoload a class here will be problematic when switching to Zeitwerk, we'll keep the `require` clause on top of the Devise initializer in order to load the file and so it will be loaded even if it isn't in the autoload paths anymore.
This we'll simplify adding these same paths to `eager_load_paths` when switching to Zeitwerk.
Just like we do with pretty much every folder.
We were getting a few errors when trying out Zeitwerk: ``` expected file lib/sms_api.rb to define constant SmsApi expected file app/components/layout/common_html_attributes_component.rb to define constant Layout::CommonHtmlAttributesComponent ``` In these cases, we aren't using an inflection because we also define the `Verification::SmsController` and a few migrations containing `Html` in their class name, and none of them would work if we defined the inflection. We were also getting an error regarding classes containing WYSIWYG in its name: ``` NameError: uninitialized constant WYSIWYGSanitizer Did you mean? WysiwygSanitizer ``` In this case, adding the acronym is easier, since we never use "Wysiwyg" in the code but we use "WYSIWYG" in many places.
Even though we don't load this file with Zeitwerk, we're doing it for consistency. If we tried to load this file using Zeitwerk, without this change we'd get an error: ``` NameError: uninitialized constant OmniauthWordpress ```
Since we're already setting `wordpress_oauth2` using the `option :name` command in the `OmniAuth::Strategies::Wordpress` class, Devise can automatically find the strategy. However, it wasn't working because we were passing a string instead of a symbol.
This way we avoid loading the OmniauthTenantSetup in the initializer, which could be problematic when switching to Zeitwerk.
In order for `include SentencesParser` to work with Zeitwerk, we'd have
to change the code slightly, so it follows Ruby conventions to resolve
constants:
```
module RemoteTranslations::Microsoft
class Client
include SentencesParser
# (...)
end
end
```
This would mean changing the indentation of the whole file. While we can
do that, changing the indentation of a file makes it harder to use
commands like `git blame` or `git log` with the file, so we're doing the
change the easy way.
This is possible since Audited 5.2.0, and will make it easier to start using Zeitwerk because it won't reference a constant in an initializer.
In Rails 6.1, the classic autoloader is deprecated. We were getting an error because we were using `autoload` in the ActiveStorage plugin for CKEditor: expected file app/lib/ckeditor/backend/active_storage.rb to define constant Ckeditor::Backend::ActiveStorage So we're removing the line causing the error. Finally, we can now restore all the tests that that failed sometimes with the classic autoloader and that we modified in commits 2af1fc7 and 8ba37b2.
While using `require_dependency` to load original Consul Democracy code from custom code works with the classic autoloader, this method was never meant to be used this way. With zeitwerk, the code (apparently) works in the test, development and production environments, but there's one important gotcha: changing any `.rb` file in development will require restarting the rails server since the application will crash when reloading. Quoting zeitwerk's author Xavier Noria, whom we've contacted while looking for a solution: > With the classic autoloader, when the Setting constant is autoloaded, > the autoloader searched the autoload paths, found setting.rb in > app/models/custom, and loaded it. With zeitwerk, the autoloader scans > the folders in order and defines an autoload (Module#autoload) in > Object so Ruby autoloads Setting with app/models/custom/settings.rb. > Later, when app/models/setting.rb is found, it's ignored since there's > already an autoload for Setting. > > That means the first file is managed by the autoloaders, while the > second is not. > > So require_dependency worked, but it was pure luck, since the purpose > of require_dependency is forcing the load of files managed by the > autoloaders and, as we've seen, app/models/settings.rb isn't one of > them. > > With your current pattern for custom files, the best solution is using > Kernel#load. So we're using `load` instead of `require_dependency`. Note that, with `load`, we need to add the `.rb` extension to the required file, and we no longer have to convert the Pathname to a string with `to_s`.
Quoting from pull request 43508 in the Rails repository [1]: > When you are running test locally, most of the time you run only a > subset, so it's better to load as little code as possible to have a > faster time to first test result. > > But when you are on CI, it's usually much preferable to eager load the > whole application because you will likely need all the code anyway, > and even if the test suite is split across runners, it's preferable to > load all the code to ensure any codefile that may have side effects is > loaded. > > This also ensure that if some autoloaded constants are not properly > tested on CI, at least they'll be loaded and obvious errors (e.g. > SyntaxError) will be caught on CI rather than during deploy. [1] rails/rails@db0ee287eed
These warnings appear in the logs in the development environment, and, with Rails 7, the application will crash. When running the tests, they would appear in the standard error ouput if we set `config.cache_classes = false` in the test environment but, since that isn't the case, they don't. To reproduce these warnings (or the lack of them), start a Rails console in development and check the log/development.log file.
Byebug hasn't been maintained for years, and it isn't fully compatible with Zeitwerk [1]. On the other hand, Ruby includes the debug gem since version 3.1.0. We tried to start using at after commit e74eff2, but couldn't do so because our CI was hanging forever in a test related to machine learning, with the message: > DEBUGGER: Attaching after process X fork to child process Y (Note this message appeared with debug 1.6.3 but not with the version we're currently using.) So we're changing the debug gem fork mode in the test so it doesn't hang anymore when running our CI. We tried to change the test so it wouldn't call `Process.fork`, but this required changing the code, and since there are no tests checking machine learning behavior with real scripts, we aren't sure whether these script would keep working after changing the code. [1] Issue 564 in https://github.com/deivid-rodriguez/byebug
taitus
approved these changes
Apr 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Notes
Huge thanks to Xavier Noria, the author of Zeitwerk, for his help with commit 6552e31 🙌 ❤️ 🙏 🎉.
Release Notes
require_dependencyin your custom code (and, if you use custom.rbfiles, you most likely do, since it's the system we've mentioned in our documentation for years) you'll have to replace it withload. See commit 6552e31 from pull request #5425 for an example showing how to do so.lib/folder to theapp/lib/folder. If you've got custom code in thelib/folder, you'll have to move it toapp/lib/as well unless you're usingrequireto load it where it's needed.