Zeitwerk-based, efficient, decorators loader#60
Closed
elia wants to merge 1 commit intosolidusio:mainfrom
Closed
Conversation
6b4c1ee to
6ba6c9b
Compare
Member
|
I like this a lot! It's not clear if the current change is backward compatible though. Do we need to change all stores/extensions to make it work once we merge/release this? |
Member
Author
|
It supports both zeitwerk and classic and can be adapted to cover existing stores and extensions. |
6ba6c9b to
eb07e8f
Compare
eb07e8f to
7b1bee3
Compare
Co-Authored-By: Andrea Longhi <andrea@spaghetticode.it>
63a72ed to
ee62cb0
Compare
1 task
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Member
|
@elia @kennyadsl is this still necessary now that we bumped Rails to >= 7.0 and a fix (#86) has been merged? |
Member
Author
|
Closing it, the diff will still be there in case anyone wants to dig it up 👍 |
mamhoff
added a commit
to mamhoff/solidus_support
that referenced
this pull request
Nov 20, 2024
This is inspired by solidusio#60. We can leverage Zeitwerk's `on_load` hook and it's capacity of knowing which constant a file should define in order to load decorators, including when reloading. This should greatly speed up reloading, as only those decorators that are needed for the current request are loaded. However, there is a few restrictions that come with this: 1. All decorators MUST use a Zeitwerk-compatible naming scheme 2. All decorators MUST use Module.prepend, where Module is the fully qualified class name being modified. Co-Authored-By: thomas@vondeyen.com
mamhoff
added a commit
to mamhoff/solidus_support
that referenced
this pull request
Nov 20, 2024
This is inspired by solidusio#60. We can leverage Zeitwerk's `on_load` hook and it's capacity of knowing which constant a file should define in order to load decorators, including when reloading. This should greatly speed up reloading, as only those decorators that are needed for the current request are loaded. However, there is a few restrictions that come with this: 1. All decorators MUST use a Zeitwerk-compatible naming scheme 2. All decorators MUST use Module.prepend, where Module is the fully qualified class name being modified. Co-Authored-By: thomas@vondeyen.com
mamhoff
added a commit
to mamhoff/solidus_support
that referenced
this pull request
Nov 20, 2024
This is inspired by solidusio#60. We can leverage Zeitwerk's `on_load` hook and it's capacity of knowing which constant a file should define in order to load decorators, including when reloading. This should greatly speed up reloading, as only those decorators that are needed for the current request are loaded. However, there is a few restrictions that come with this: 1. All decorators MUST use a Zeitwerk-compatible naming scheme 2. All decorators MUST use Module.prepend, where Module is the fully qualified class name being modified. Co-Authored-By: thomas@vondeyen.com
mamhoff
added a commit
to mamhoff/solidus_support
that referenced
this pull request
Nov 21, 2024
This is inspired by solidusio#60. We can leverage Zeitwerk's `on_load` hook and it's capacity of knowing which constant a file should define in order to load decorators, including when reloading. This should greatly speed up reloading, as only those decorators that are needed for the current request are loaded. However, there is a few restrictions that come with this: 1. All decorators MUST use a Zeitwerk-compatible naming scheme 2. All decorators MUST use Module.prepend, where Module is the fully qualified class name being modified. Co-Authored-By: thomas@vondeyen.com
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This work is built on the awesome work by @fxn on zeitwerk for Rails and @aldesantis on prependers for Solidus
The code in this PR has been tried successfully on a couple of real-world projects with multi-year legacy. The basic principle is that we should only load the decorators for the base classes that are required for the current request, finally playing nice with Rails own autoloading/reloading mechanisms.
One thing that was very interesting is seeing classes autoloaded from initializers (which is an anti-pattern) and the ripple effect they create in terms of loading their dependencies, fixing that kind of stuff would make Solidus stores startup virtually as fast as a pristine Rails app.
As a side note this work will be also proposed for the Rails guides that currently suggest eagerly loading all the decorators, we think, for lack of a better alternative.
from the inline documentation