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

refactor: better engine mounting #3533

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

adrianthedev
Copy link
Collaborator

@adrianthedev adrianthedev commented Dec 15, 2024

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.

# Other engines use this API to self-mount themselves
Avo.plugin_manager.mount_engine Avo::Collaborate::Engine, at: "/collaborate"

# like so...
module Avo
  module Collaborate
    class Engine < ::Rails::Engine
      isolate_namespace Avo::Collaborate

      initializer "avo-collaborate.init" do
        ActiveSupport.on_load(:avo_boot) do
          Avo.plugin_manager.register :avo_collaborate

          Avo.plugin_manager.mount_engine Avo::Collaborate::Engine, at: "/collaborate"
        end
      end
    end
  end
end

TODO

  • make mount_avo take a at: argument
  • cleanup
  • tests
  • update all other plugins to self-mount themselves
  • docs

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

codeclimate bot commented Dec 15, 2024

Code Climate has analyzed commit a3cf5ce and detected 0 issues on this pull request.

View more on Code Climate.

lib/avo/plugin_manager.rb Outdated Show resolved Hide resolved
lib/avo/plugin_manager.rb Outdated Show resolved Hide resolved
lib/avo/engine.rb Show resolved Hide resolved
lib/avo/engine.rb Show resolved Hide resolved
@@ -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
Copy link
Collaborator Author

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

@Paul-Bob Paul-Bob marked this pull request as ready for review February 11, 2025 13:27
@Paul-Bob
Copy link
Contributor

Upgrade guide:

  • Remove all references to mount_avo_engines
  • Remove all references to Avo.mount_engines
  • Replace mount Avo::Engine, at: Avo.configuration.root_path with mount_avo

@@ -39,12 +44,6 @@ class Engine < ::Rails::Engine
end
end

# Ensure we reboot the app when something changes
config.to_prepare do
Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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

mount Avo::Engine, at:, **options

Avo.plugin_manager.engines.each do |engine|
mount engine[:klass], at: "#{at}/#{engine[:options].delete(:at)}", **engine[:options]
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@@ -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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants