-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feat(Performance): Use Zeitwerk for loading decorators #87
Conversation
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 is so exciting!
# Multiple hooks are no problem, as long as all decorators are namespaced appropriately. | ||
autoloader.on_load(decorated_class) do |base| | ||
Rails.logger.debug("Loading #{decorator_constant} in order to modify #{base}") | ||
decorator_constant.constantize |
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.
Is there a Zeitwerk way to constantize or "this is the way"?
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.
It seems there is on_setup
https://github.com/fxn/zeitwerk/blob/main/CHANGELOG.md?plain=1#L322-L332
Can we try that?
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.
Reading the Zeitwerk README
https://github.com/fxn/zeitwerk#eager-load-directories
Can we simply append root.join("app/decorators")
to loader.eager_load_dir
and get rid of all this custom code? I am pretty sure Zeitwerk is able to do all the heavy lifting and deal with edge case for us, no?
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 could do that, and that would reimplement the load
stuff we've been doing up to now. However: Decorators are "contagious" in that when they are loaded, they also trigger autoloading of the classes they reference, along with all the classes mentioned in those classes. What this PR does is help keep us completely away from autoloadable code during startup.
1726611
to
cca2824
Compare
@@ -46,14 +49,42 @@ def load_solidus_subscribers_from(path) | |||
end | |||
end | |||
|
|||
# Loads decorator files. | |||
# Loads decorators. |
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've started the move to using "overrides" instead of "decorators" (because that's what the Rails docs call them and also this isn't the decorator pattern.) Do we want to take this continue moving that direction?
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.
For reference this has been discussed before in solidusio/solidus#3010
Rephrasing my previous comment from above mentioned issue on "overrides" here:
But, I wholeheartedly disagree with calling them
*_overrides.rb
. These things are monkey patches. Please, let's call them what they are then (*_monkey_patch.rb
s). Everybody knows what a monkey patch is. We even call them monkey patches every time we explain what these "decorators" are meant for. So, why another disguise?Also, Deface, a still widely used gem in Solidus ecosystem, already uses the
app/overrides
folder*.
*) solidusio/solidus#4877 is still an issue because of the wide spread usage of Deface in our eco system.
Not sure I would still call them "monkey patches" nowadays (it's kind of "toy language" and sounds like something has been desperately made out of emergency reasons). Since this pattern is so common in our eco system I would like to give it a better, more formal name. Ruby itself coined the term "Refinements", but we cannot use them for obvious reasons.
So, what are we left with? "Extensions", "Prependers" (as mentioned in #60), "Overloads"?
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.
Just found https://github.com/nebulab/prependers
Why has this not been adopted for Solidus yet?
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.
overrides
is problematic, because the Deface gem uses load
to load things in app/overrides
, regardless of whether it's a deface DSL file or any other Ruby file. I can imagine using app/patches
instead: That would be less derisive as a name, but still indicate patching core classes is not entirely desired.
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.
I really want to follow Rails conventions on this, but understand the impasse. I propose we hold off on changing anything as part of this PR and come up with a good solution later. (Sorry for bringing it up! I typically just use the Deface DSL and hadn't run into these issues!)
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 make a Rails convention then :) rails/rails#53714
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.
The Rails team doesn't like patches, and would prefer not to encourage people. I'll try getting something together that implements more or less what's in that PR in a separate gem, usable for the Solidus ecosystem, the Alchemy ecosystem, and, importantly, for host apps.
While I understand the arguments about patches being a bad practice, I also understand they're an extremely powerful tool that we all often reach for, and should therefore support in a way that works well.
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.
Thanks @mamhoff. That sounds great to me.
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: [email protected]
cca2824
to
ca748df
Compare
The Rails guides on engines were for the most part written by members of our community. So, I would take this with a grain of salt. It is not an official Rails core team recommendation
|
Replaced with #90 |
This is inspired by #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:
Co-Authored-By: [email protected]
This is a draft for now, but we've seen it work before.