Merged
Conversation
chaadow
reviewed
Jun 25, 2023
| raise e, "Cannot load database configuration:\n#{e.message}", e.backtrace | ||
| end | ||
|
|
||
| def autoload_lib(ignore:) |
Contributor
There was a problem hiding this comment.
@fxn Maybe by calling this method, we can also remove lib from Ruby's load path
rails/railties/lib/rails/application.rb
Line 387 in 5c5b78d
only if this option is set to false
Member
Author
There was a problem hiding this comment.
@chaadow good call. I think this one is tricky:
- People may expect
libto be in$LOAD_PATHbecause it is generally there in Ruby projects. - Rails issues a
requireto load generators on demand.lib/generatorsis idiomatic and you'd expect that to work as it did until now.
Let me think about it. Even if we special-case it, it'd need some docs and tests, so the comment was useful in either case, thanks!
Member
Author
There was a problem hiding this comment.
In the end, I opted for documenting and adding test coverage for it (#48596). Thanks!
4 tasks
This file contains hidden or 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
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.
Introduction
This patch introduces API to easily autoload and eager load from
lib:Some history
The
libdirectory was in the autoload paths by default in the early versions of Rails. However,libstores different kinds of files, and eager loading in production proved to be an issue. Some files in there are not meant to be autoloaded or eager loaded. Think generators, for example.Because of that,
libwas removed from the default autoload paths in Rails 3.app/lib
Since
libdidn't have a good solution, the idea of usingapp/libstarted to emerge.New
appsubdirectories are added to the autoload and eager load paths automatically, and a posteriori you put there only regular code (not assets or tasks, for example), it worked out of the box.However, I have a few remarks about
app/lib:app/modelsandapp/controllersmean something,app/libmeans nothing. They are at the same level, but naming is not quite consistent.libis meant for things that are kind of tangential to the application core. What's in there feels better located inlibthan underapp, for me.lib.libis added to the autoload paths by hand anyway. People want to autoload fromlib, which is understandable. If we can, this use case should have first-class support from the framework.We have more options now
Zeitwerk is able to ignore files and directories, so now Rails programmers can autoload and eager load from
libwith more control. For example:Technically, that is possible since Rails 6, but a better API is possible.
config.autoload_lib(ignore:)In API design, exceptional use cases may justify exceptional support. You design for the common case, and let the edge case be edge.
In this case, I believe
libdeserves ad-hoc API that allows users to do exactly that in one shot:Values passed to
ignoreare relative tolibbecause it does not make sense to ignore anything outsidelibusing this API, so you enforce that implicitly. As a bonus, the call is short and concise.Why no default arguments?
I wondered if those three should be a default, but look at this:
Is it obvious that there are directories being ignored? No, you open the file and see "autoload lib". That would be a misleading API. I believe a default makes sense when it is a no-brainer for clients, but in this case I don't think it is.
In this case, I believe it is better to be explicit and require that you think about what to ignore. Documentation already takes you there by example.
Why not generated for new applications?
I believe we could eventually include that line in newly generated applications. However, I don't want to rush this, let's have the API out and see. If it works well, perhaps in a future version of Rails.
Why not available for engines?
The
libdirectory in engines is generally speaking more similar to thelibdirectory of a gem than to thelibdirectory of an application. Havinglibreloaded in engines is more tricky. By now, I prefer to let people that know what they do configure the autoloaders by hand.