-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load constants lazily on RailsAdmin initialization #3541
Merged
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
This partially reverts commit e4ae669, except for reload-related part.
… loading the actual classes
codealchemy
reviewed
Jul 12, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this change addresses the issues I was running into in #3534
Some thoughts re: allowing model constants to be used in the initializer.
codealchemy
approved these changes
Jul 14, 2022
Merged. Thank you for reviewing! |
This was referenced Jul 18, 2022
This is working well in my testing and resolves #3490. Thank you for figuring this out! |
mshibuya
added a commit
that referenced
this pull request
Aug 12, 2022
mshibuya
added a commit
that referenced
this pull request
Aug 12, 2022
wilg
added a commit
to wilg/rails_admin
that referenced
this pull request
Sep 14, 2022
…ller set" This reverts commit 33e9214.
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.
This is a counter-proposal for #3492, and aims to fix issues #3490, #3501, #3533, and #3534.
My apologies for those issues. This is because of the complexity in RailsAdmin (and Rails) initialization process, which is hard to get it right. Let me explain the story...
RailsAdmin initializer (
config/initializers/rails_admin.rb
) is loaded during the Rails app's startup process. But as for the actual configuration (= execution of the DSL itself, the code passed in theRailsAdmin.config do |config| ... end
block), there're many constraints that all need to be fulfilled for a successful initialization.app/
directory) are not loaded yet.config.model
argument. This led to issues like Active Storage:method_missing': undefined method
has_one_attached' #3025 in pre-Zeitwerk age, and with Zeitwerk it's not possible because its autoload feature is not enabled at this moment.So the attempt to address this was made in e4ae669. The idea was basically to delay the execution of the configuration block, after model classes are ready to be autoloaded. But it brought different problems:
reload_routes!
onbefore_eager_load
. That means RailsAdmin actions (which are used to construct RailsAdmin routes) need to be set up ahead of that, unless RailsAdmin routes fail to be reflected. To work aronud this issue,reload_routes!
was also added in the RailsAdmin initialization process. And that solution isn't perfect, resulting in Reloading Routes before all cause Devise initializers not to be loaded dependent to the order of gem requires #3533.after_initialize
(at that point, it's too late to modify values inapp.config.assets
). So the configuration execution was brought earlier, but that didn't go well and brought Unable to precompile assets when updating to3.0.0.beta
#3534.Thus, it's like 2 contradicting forces pulling the initialization timing both forward ant backward. That's why I have been struggling in solving this issue without success...
We need a new way to overcome this problem, and here I have come up with one. That is to intercept
const_missing
to eliminate the necessity for loading actual classes, while delaying the model initialization by utilizingLazyModel
, which is the proven method from pre RailsAdmin 3.x era.At the first sight overriding
Object.const_missing
seemed to have side effects, but from what I tested it looks working well. So I want you guys to try this in your setup, and give feedback on whether this is good idea or not.Thank you for reading this through, and hoping to hearing back!
@codealchemy @jdufresne @q3aiml (or anyone who is interested in this)