Use Zeitwerk to autoload files #5425

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Zeitwerk to autoload files #5425

Use Zeitwerk to autoload files #5425

Commits on Apr 11, 2024