-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
refactor: better engine mounting #3533
base: main
Are you sure you want to change the base?
Conversation
Code Climate has analyzed commit a3cf5ce and detected 0 issues on this pull request. View more on Code Climate. |
@@ -6,10 +6,6 @@ Avo.configure do |config| | |||
# used only when you have custom `map` configuration in your config.ru | |||
# config.prefix_path = "/internal" | |||
|
|||
# Sometimes you might want to mount Avo's engines yourself. | |||
# https://docs.avohq.io/3.0/routing.html | |||
# config.mount_avo_engines = true |
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.
This should still be here with a warning that it's not working anymore.
Deprecation warning.
Maybe
Upgrade guide:
|
@@ -39,12 +44,6 @@ class Engine < ::Rails::Engine | |||
end | |||
end | |||
|
|||
# Ensure we reboot the app when something changes | |||
config.to_prepare do |
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.
don't we still need this for reloading the code?
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.
Let's run this only in development
lib/avo/engine.rb
Outdated
mount Avo::Engine, at:, **options | ||
|
||
Avo.plugin_manager.engines.each do |engine| | ||
mount engine[:klass], at: "#{at}/#{engine[:options].delete(:at)}", **engine[:options] |
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.
We need comments with examples.
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.
And maybe use scope at do
@@ -39,12 +44,6 @@ class Engine < ::Rails::Engine | |||
end | |||
end | |||
|
|||
# Ensure we reboot the app when something changes | |||
config.to_prepare do |
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.
Let's run this only in development
lib/avo/engine.rb
Outdated
@@ -21,6 +21,11 @@ class Engine < ::Rails::Engine | |||
isolate_namespace Avo | |||
|
|||
config.after_initialize do | |||
# This callback is triggered 2 times | |||
# This flag check will avoid to re-execute the logic | |||
next if @already_initialized |
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.
We don't need this in production.
Development will always run it twice.
@@ -112,7 +111,6 @@ def initialize | |||
@field_wrapper_layout = :inline | |||
@resources = nil | |||
@resource_parent_controller = "Avo::ResourcesController" | |||
@mount_avo_engines = true |
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.
Let's add a setter method to raise an error with docs link.
@@ -133,17 +133,6 @@ def has_profile_menu? | |||
true | |||
end | |||
|
|||
# Mount all Avo engines | |||
def mount_engines |
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.
Same. Raise and error with docs links.
Description
This PR refactors the way the Avo engine is mounted. Same for other Avo plugins.
This should significantly improve the way we recommend users to mount Avo when they are using the
scope
option.I guess we could even remove the
self.mount_avo_engines
configuration option at some point.TODO
mount_avo
take aat:
argumentChecklist:
Screenshots & recording
Manual review steps
Manual reviewer: please leave a comment with output from the test if that's the case.