Skip to content
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

restore 2.x config loading behavior #3492

Closed

Conversation

q3aiml
Copy link
Contributor

@q3aiml q3aiml commented Mar 10, 2022

Attempt to restore 2.x config loading behavior where 1) per-model config blocks
only evaluate after all the models are loaded and associations are defined
while 2) initializer config block evaluates immediately so that routes are
configured correctly.

One remaining change compared to 2.x is RailsAdmin::Config#model no longer
returns the model when given a block. With the removal of LazyModel in e4ae669
it is tricky to both defer initialization and return the model class.

Fixes #3490

Attempt to restore 2.x config loading behavior where 1) per-model config blocks
only evaluate after all the models are loaded and associations are defined
while 2) initializer config block evaluates immediately so that routes are
configured correctly.

One remaining change compared to 2.x is RailsAdmin::Config#model no longer
returns the model when given a block. With the removal of LazyModel in e4ae669
it is tricky to both defer initialization and return the model class.

Fixes railsadminteam#3490
@q3aiml q3aiml force-pushed the restore-deferred-config-loading branch from 5696edb to ec03eb6 Compare March 10, 2022 23:07
@mshibuya
Copy link
Member

Thanks for trying! I like your idea, but

  1. initializer config block evaluates immediately

there's a problem with this part. This will cause the issue with autoloading.

Sorry not covering it in the spec for now, I was unable to reproduce from the spec it since it happens during the initial loading stage. But you can reproduce it by using the dummy app, with changing the initializer as

 RailsAdmin.config do |c|
   c.asset_source = CI_ASSET
-  c.model 'Team' do
+  c.model Team do
     include_all_fields
     field :color, :color
   end

and starting the Rails app. You will get uninitialized constant Team error, since the Rails autoloader is not set up yet on the execution of the RailsAdmin initializer. But this notation c.model <a model class> is totally valid in the current DSL.
Hence my choice was to defer initialization to some extent...

Could you see if this is fixable?

q3aiml added a commit to q3aiml/rails_admin that referenced this pull request Mar 25, 2022
In 3.x the initializer config blocks are intentionally deferred to allow
referencing model classes without quoting. Here is a first pass at keeping that
behavior on top of ec03eb6. It could probably use a little cleanup but here's
an idea of an approach for feedback.

See
railsadminteam#3492 (comment)
for details.
In 3.x the initializer config blocks are intentionally deferred to allow
referencing model classes without quoting. Here is a first pass at keeping that
behavior on top of ec03eb6. It could probably use a little cleanup but here's
an idea of an approach for feedback.

See
railsadminteam#3492 (comment)
for details.
q3aiml added a commit to q3aiml/rails_admin that referenced this pull request Mar 25, 2022
In 3.x the initializer config blocks are intentionally deferred to allow
referencing model classes without quoting. Here is a first pass at keeping that
behavior on top of ec03eb6. It could probably use a little cleanup but here's
an idea of an approach for feedback.

See
railsadminteam#3492 (comment)
for details.
q3aiml added a commit to q3aiml/rails_admin that referenced this pull request Mar 25, 2022
In 3.x the initializer config blocks are intentionally deferred to allow
referencing model classes without quoting. Here is a first pass at keeping that
behavior on top of ec03eb6. It could probably use a little cleanup but here's
an idea of an approach for feedback.

See
railsadminteam#3492 (comment)
for details.
@q3aiml q3aiml force-pushed the restore-deferred-config-loading branch from e6e4dfd to 00ac334 Compare March 25, 2022 20:41
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.964% when pulling 00ac334 on q3aiml:restore-deferred-config-loading into 9c72851 on railsadminteam:master.

@q3aiml
Copy link
Contributor Author

q3aiml commented Mar 25, 2022

Thanks for the review! I didn't think about how that change was intentional with 3.x (unless I am mistaken I think the autoload warning/error exists as well in 2.x when using c.model <a model class> in the initializer). After reviewing issues like #3025 I can see how it could be less confusing to defer the initializer config too.

I made a first attempt at a fix. The idea is to have two separate lists of deferred config blocks: one for the initializer config (I called it "core"??) and one for the model config. It probably deserves a little cleanup but I want to get it out for your thoughts before getting too far along. Currently it does change some methods that could be considered public api with the release of 3.0.0 so that might deserve some rework as well.

@jeromedalbert
Copy link

jeromedalbert commented Mar 30, 2022

Thanks for making this PR, I am having class loading issues on staging/production when I use rails_admin 3.0.0 along with the makara gem, and using your branch fixes it. I hope your PR gets merged.

@mshibuya
Copy link
Member

Sorry for not getting back for a long time, I needed some time to consider all implications of the initialization process.
And I came to realize that the change proposed in this PR is not compatible with the Importmap support #3488, which requires asset_source to be set earlier than after_initialize.

So I'm proposing another way to address this as #3541. It would be great if you could take a look.

@q3aiml
Copy link
Contributor Author

q3aiml commented Jul 18, 2022

Thanks for circling back with the update! It's definitely a tricky problem. And good catch on the importmap incompatibility.

At first look #3541 makes sense to me. I'll give it a try shortly.

@q3aiml q3aiml closed this Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

per model config can no longer be defined before associations
4 participants