autobind-decorator
autobind-decorator copied to clipboard
Fix shared instance fields
This PR makes pretty big changes
Some of them will have performance impact, like binding every time when we call getter on super
(see comment in code)
Both: setter and getter of original descriptor may now create instance-bound property: though actual binding will happen on real get (callGetter argument of reDefineProperty)
Thanks for the PR!
Updated PR, simplified tests
Removed code that auto-binds super method. Correct solution seems to be more complicated and tends to be over-engineered.
Will live my thoughts on this here:
- Caching is not straightforward, because method is defined on prototype chain and we should not shadow instance level method, which may potentially happen, i.e. we have class hierarchy A -> B -> C all having method with name
getValue
annotated with@boundMethod
, if it is called on C instance how should we cache bound methods on A & B, how to construct cache key? Options may be some integer counter, or depth of parent in chain - Changing method on parent’s prototype should or should not work? Should we reset cached bound version? (I think that yes, we should)
- Caching is not straightforward, because method is defined on prototype chain and we should not shadow instance level method, which may potentially happen, i.e. we have class hierarchy A -> B -> C all having method with name
getValue
annotated with@boundMethod
, if it is called on C instance how should we cache bound methods on A & B, how to construct cache key? Options may be some integer counter, or depth of parent in chain- Changing method on parent’s prototype should or should not work? Should we reset cached bound version? (I think that yes, we should)
What about the performance? How hard is it to implement?
This will impact performance for classes that have boundMethods
on parents
bind classes is a bad idea anyway... As long as it doesn't impact methods thats fine.
Hi @betalb , could you please add the caveats of new behaviours in README? And then we'll merge this :)
Sorry, was caught with other things, will try to finish everything by the end of the year
Thanks for your works, when will this be merged?
As long as docs is added I'll merge this.