Fix shared instance fields

[betalb]

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)

[stevemao]

This test seems too much. Why can't we just change a method on a and see if that method is the same on b?

[stevemao]

I think we also need a test:
Once instance method is changed, it shouldn't be bound.

[betalb]

For the first point: don't think that this will work, methods won't be equal, they will be bound instances of the same underlying function. So theoretically we need to check underlying method. Though I think that test case may be made cleaner. And just realised that

const aOriginalGetValue = a.getValue;
a.getValue = function () {
  return 'foo' + '-' + aOriginalGetValue();
};

And

a.getValue = function () {
  return 'fake-foo';
};

Will hit different code paths, will need to cover this too

For the second point: this one is interesting. Are you saing that if we run setter with new function it shouldn't be auto-bound? If yes, this is not how existing code works (before my change), so I've mimicked this behaviour

[stevemao]

For the second point: this one is interesting. Are you saing that if we run setter with new function it shouldn't be auto-bound?

What do you think? I feel like autobind is bound on the method not the property. Let's also take performance into consideration.

[betalb]

I agree, we should not continue auto-binding if somebody changes method, or more interesting - plays with prototype chain of parents

But this will be breaking change and should be done as separate PR

[stevemao]

But this will be breaking change and should be done as separate PR

Since this change is quite big I'd release a major for this anyways...

[stevemao]

we should not continue auto-binding if somebody changes method, or more interesting - plays with prototype chain of parents

Will this improve the performance?

[betalb]

In certain use-cases, most likely, but this needs to be proved

[stevemao]

Thanks for the PR!

[betalb]

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)

[stevemao]

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

[betalb]

This will impact performance for classes that have boundMethods on parents

[stevemao]

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

[betalb]

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

[mr-pinzhang]

Thanks for your works, when will this be merged?

[stevemao]

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