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

Load Solidus engine extension files automatically #42

Merged
merged 2 commits into from
Feb 3, 2020

Conversation

aldesantis
Copy link
Member

@aldesantis aldesantis commented Jan 31, 2020

Improves EngineExtensions::Decorators to automatically load the right files when Solidus engines are installed. For instance, if solidus_backend is installed, it will tell Rails to load lib/views/backend and lib/controllers/backend, as well as the decorators in lib/decorators/backend.

This also renames SolidusSupport::EngineExtensions::Decorators to SolidusSupport::EngineExtensions, since the module doesn't only deal with decorators anymore.

@aldesantis aldesantis requested a review from kennyadsl January 31, 2020 14:34
@aldesantis aldesantis self-assigned this Jan 31, 2020
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super! Just left a couple of things.

lib/solidus_support/engine_extensions/decorators.rb Outdated Show resolved Hide resolved
lib/solidus_support/engine_extensions/decorators.rb Outdated Show resolved Hide resolved
@aldesantis aldesantis force-pushed the feature/enable-engines-conditionally branch from af42b93 to 061a674 Compare January 31, 2020 14:53
@spaghetticode
Copy link
Member

@aldesantis I have one concern.

We're adding new private methods to a class that inherits form Rails::Engine, a class that we don't own. I wonder if the new methods, having a rather generic name, may end up in overriding the ones that could be defined by the superclass in a remote but still plausible future.

So, instead of for example decorators_root, would something like solidus_decorators_root make more sense?

@aldesantis aldesantis changed the title Enable support for Solidus engines conditionally Load Solidus engine files automatically Jan 31, 2020
@aldesantis
Copy link
Member Author

@spaghetticode I think that's a good point. It seems unlikely that Rails will officially introduce the concept of engines at some point, but it doesn't hurt to give the methods names that are a bit more specific.

@aldesantis aldesantis force-pushed the feature/enable-engines-conditionally branch 2 times, most recently from f5f0aab to 3ebfcbc Compare January 31, 2020 14:59
@aldesantis aldesantis changed the title Load Solidus engine files automatically Load Solidus engine extension files automatically Jan 31, 2020
@aldesantis aldesantis force-pushed the feature/enable-engines-conditionally branch 4 times, most recently from 86e0126 to 3bdaf27 Compare January 31, 2020 15:20
@aldesantis aldesantis force-pushed the feature/enable-engines-conditionally branch 4 times, most recently from 135f486 to ce65e35 Compare January 31, 2020 15:35
By cleanly separating extension files for the different Solidus
engines, we can only load solidus_backend files when the engine
itself is available rather than doing it all the time.

In turn, this allows extensions to only actually require the parts
of Solidus that they really need.
@aldesantis aldesantis force-pushed the feature/enable-engines-conditionally branch from ce65e35 to 0073fc6 Compare January 31, 2020 15:38
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aldesantis thank you 👍

This module doesn't just deal with decorators anymore, but also
loads views and controllers, so this seems like a better name.
Separating it into two modules wouldn't be simple because they
are interlinked.
@aldesantis aldesantis force-pushed the feature/enable-engines-conditionally branch from a2379e3 to 947778a Compare January 31, 2020 16:58
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C O O L!

@aldesantis aldesantis merged commit cd4892d into master Feb 3, 2020
@aldesantis aldesantis deleted the feature/enable-engines-conditionally branch February 3, 2020 10:42
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.

3 participants