-
Notifications
You must be signed in to change notification settings - Fork 25
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
Change prepend MemoWise
to extend MemoWise
#253
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #253 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 3 +1
Lines 166 144 -22
Branches 83 67 -16
=========================================
- Hits 166 144 -22
Continue to review full report at Codecov.
|
f80b76e
to
881beaa
Compare
e7b1a47
to
81b2ff0
Compare
81b2ff0
to
4057920
Compare
Co-authored-by: Jemma Issroff <[email protected]>
…ynamically generated
4057920
to
a5b4743
Compare
So what's up with this here PR? I'm looking to get working with Sorbet in my app and it's straight incompatible with |
Hi @dhnaranjo @halo! Sorry I missed the question above! We had paused this work when we found it wasn't as performant in our benchmarks and we were unable to figure out why. I'm in the process of upgrading our benchmarks to Ruby 3.2 and will take a look at how this branch performs under 3.2 once that's done! While I don't use Sorbet, I do want this gem to be usable by folks who do. |
Co-authored-by: Jemma Issroff [email protected]
What the refactor does:
The only public facing change in this refactor is that instead of
prepend MemoWise
, clients will nowextend MemoWise
. Internally, this refactor simplifies the machinery necessary to get memoization working, including eliminating back and forth of singleton classes and deferring definitions ofpreset_memo_wise
andreset_memo_wise
. With this change, MemoWise will also no longer redefine methods, simplifying delegation to the original method and making it easier to determine which methods are memoized.Guiding principles for the refactor:
MemoWise
module should be as nonintrusive as possible. By utilizingextend
, we make sure that mixing inMemoWise
only adds thememo_wise
method on the the class, which is the main API of the library.initialize
) and eagerly defines many instance variables on the mixin site, thus unnecessarily polluting the target.ObjectSpace
to find attached objects to singleton classes, or the library needs to keep checking if the mixin site is a singleton class or not.super
to delegate to the actual instance. Moreover, this method of prepended methods plays nicer with other forms of method wrapping that other libraries could be using as well (for example, in its current form MemoWise would not play nicely with Sorbet runtime).How the refactor works:
Step 1. Call
extend MemoWise
from a class/module.This exposes the method
memo_wise
to the class/module as a class method.Step 2. Call the
memo_wise
method on an instance method (for example,memo_wise :example_method
).With the first
memo_wise
method call, a module namedMemoWiseMethods
is created. This module haspreset_memo_wise
,reset_memo_wise
andfreeze
as instance methods. Thememo_wise
call also adds a memoized shadow instance method on this module (for example,MemoWiseMethods#example_method
).Step 3. The
MemoWiseMethods
module is automaticallyprepend
ed on the original class/module.The use of
prepend
means that the shadow methods on this module take precedence over the original methods. Callingsuper
from within the shadow methods is sufficient for accessing the original methods (instead of doing the "alias-method-chain" dance).Notes:
memo_wise
on class methods makes the singleton class of the class/moduleextend MemoWise
, and callsmemo_wise
for the instance methods on the singleton class. Accordingly,preset_memo_wise
andreset_memo_wise
are only defined as class methods the first timememo_wise
is called on a class method (withself:
).freeze
method and initializing internal state before callingsuper
.Before merging:
README.md
and update this PRCHANGELOG.md
, add an entry following Keep a Changelog guidelines with semantic versioning