autobind-decorator icon indicating copy to clipboard operation
autobind-decorator copied to clipboard

Published 20 hours ago • verified publisher icon andreypopp

Fix shared instance fields

Open betalb opened this issue 5 years ago • 9 comments

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)

betalb avatar Nov 20 '18 12:11 betalb

Thanks for the PR!

stevemao avatar Nov 20 '18 12:11 stevemao

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:

  1. 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
  2. Changing method on parent’s prototype should or should not work? Should we reset cached bound version? (I think that yes, we should)

betalb avatar Nov 21 '18 06:11 betalb

  • 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?

stevemao avatar Nov 21 '18 22:11 stevemao

This will impact performance for classes that have boundMethods on parents

betalb avatar Nov 23 '18 06:11 betalb

bind classes is a bad idea anyway... As long as it doesn't impact methods thats fine.

stevemao avatar Nov 23 '18 12:11 stevemao

Hi @betalb , could you please add the caveats of new behaviours in README? And then we'll merge this :)

stevemao avatar Dec 20 '18 00:12 stevemao

Sorry, was caught with other things, will try to finish everything by the end of the year

betalb avatar Dec 20 '18 11:12 betalb

Thanks for your works, when will this be merged?

mr-pinzhang avatar Jan 18 '19 04:01 mr-pinzhang

As long as docs is added I'll merge this.

stevemao avatar Jan 18 '19 04:01 stevemao