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

Stop generating code to autoload overrides #4231

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

waiting-for-dev
Copy link
Contributor

Up to this PR, when installing Solidus, we're adding code to the host
application to load files matching app/**/*_decorator*.rb.

There exist a series of issues about that:

For all of that, it's something that we should address in the docs.

This change is backward compatible, as the running code doesn't belong
to the Solidus engine but to the previously generated applications,
which will keep it after upgrading.

The deleted code is, in fact, intended to work with the classic
autoloader (removed on Rails 7). However, it still works because of
https://github.com/rails/rails/blob/296ef7a17221e81881e38b51aa2b014d7a28bac5/activesupport/lib/active_support/dependencies/require_dependency.rb,
which is deprecated. We add a notice to the CHANGELOG to let users known
about that.

On a related information, the recommended app/overrides/ directory is
the same used by deface (https://github.com/spree/deface), a common
dependency in Solidus projects. However, both types of overrides can
coexist without interfering with each other (see
#3010 (comment)).
We decided to tackle the issue upstream on deface (see
#3010 (comment)).

Closes #3010

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@waiting-for-dev waiting-for-dev requested a review from a team December 27, 2021 13:05
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.

Thanks, @waiting-for-dev. Is there anything in the current Guides (the ones in /guides) that we can change accordingly within this PR?

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/stop_autoloading branch 4 times, most recently from 8099f29 to e6df250 Compare December 28, 2021 05:31
@waiting-for-dev
Copy link
Contributor Author

Thanks, @waiting-for-dev. Is there anything in the current Guides (the ones in /guides) that we can change accordingly within this PR?

Good point, @kennyadsl. I just updated them. I renamed the decorators.html.md file to monkey_patches.html.md, and I changed the wording to use monkey patch instead of decorator. Would it be possible to create a 301 on the server for the old page (so that SEO, bookmarks... are not affected)?

@kennyadsl
Copy link
Member

Would it be possible to create a 301 on the server for the old page (so that SEO, bookmarks... are not affected)?

I think it's possible somehow (https://forum.middlemanapp.com/t/netlify-redirects-file/2635). Still, I'd like to explore keeping that page because the changes proposed are still not in any released version and also most of the people reading the documentation won't update immediately. What if we add a disclaimer to the old page instead, telling people that this is a valid behavior until version < 3.2, and to take a look at the other page for newer versions?

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/stop_autoloading branch 2 times, most recently from a00c063 to 9c30de5 Compare December 28, 2021 11:00
@waiting-for-dev
Copy link
Contributor Author

Still, I'd like to explore keeping that page because the changes proposed are still not in any released version and also most of the people reading the documentation won't update immediately. What if we add a disclaimer to the old page instead, telling people that this is a valid behavior until version < 3.2, and to take a look at the other page for newer versions?

I think you're right. Please, take a look at the disclaimer I added.

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.

Thanks Marc! We should probably also update the Edge Guides and start thinking at a way to make extensions respect this new way of creating monkey patches. But none of these things have to block this PR.

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/stop_autoloading branch from 9c30de5 to 9d3a180 Compare January 5, 2022 05:03
CHANGELOG.md Show resolved Hide resolved
@@ -48,8 +48,8 @@
href: "/developers/customizations/customizing-storefront.html"
- title: "Customizing the Admin Panel"
href: "/developers/customizations/customizing-admin.html"
- title: "Decorators"
href: "/developers/customizations/decorators.html"
- title: "Monkey patches"
Copy link
Contributor

Choose a reason for hiding this comment

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

Patches should probably be capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/stop_autoloading branch from 9d3a180 to 3ce59f7 Compare January 7, 2022 04:40
Up to this PR, when installing Solidus, we're adding code to the host
application to load files matching `app/**/*_decorator*.rb`.

There exist a series of issues about that:

- It goes against current recommendations on the Rails guides, where
overrides are placed under `app/overrides/**/_*override.rb` ( see
https://guides.rubyonrails.org/engines.html#improving-engine-functionality).

- The `_decorator.rb` suffix is misleading, as that kind of overrides
has to be seen as monkey patches and not related to the decorator
pattern (see https://en.wikipedia.org/wiki/Decorator_pattern).

- Although it's something needed a lot of times, monkey patching core
classes should be seen as a last resort after ruling out other
strategies (like configuring a custom class for some behavior). As such,
we should not encourage it.

- It may conflict with the naming used by other gems, like Draper
(https://github.com/drapergem/draper).

For all of that, it's something that we should address in the docs.

This change is backward compatible, as the running code doesn't belong
to the Solidus engine but to the previously generated applications,
which will keep it after upgrading.

The deleted code is, in fact, intended to work with the classic
autoloader (removed on Rails 7). However, it still works because of
https://github.com/rails/rails/blob/296ef7a17221e81881e38b51aa2b014d7a28bac5/activesupport/lib/active_support/dependencies/require_dependency.rb,
which is deprecated. We add a notice to the CHANGELOG to let users known
about that.

On a related information, the recommended `app/overrides/` directory is
the same used by `deface` (https://github.com/spree/deface), a common
dependency in Solidus projects. However, both types of overrides can
coexist without interfering with each other (see
#3010 (comment)).
We decided to tackle the issue upstream on `deface` (see
#3010 (comment)).

Closes #3010
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/stop_autoloading branch from 3ce59f7 to 56bc7fc Compare January 7, 2022 04:41
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.

RFC: Change *_decorator pattern
7 participants