Skip to content
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

If bound methods or fields are re-assigned, they become shared between instances #76

Open
betalb opened this issue Nov 16, 2018 · 8 comments

Comments

@betalb
Copy link

betalb commented Nov 16, 2018

If bound methods or fields are re-assigned, they become shared between instances

Found this issue while trying to debounce bound method

Below test will fail

class A {
  @boundMethod
  bound() {}
}

const a = new A();
const b = new A();
a.bound = {
  foo: 'bar'
};
assert.notEqual(a.bound, b.bound);
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 16, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 16, 2018
@betalb
Copy link
Author

betalb commented Nov 16, 2018

Committed failing test cases and fix for @boundMethod decorator

@stevemao
Copy link
Collaborator

Does it happen if you use @autobind (the old default exported module)?

@stevemao stevemao added the bug label Nov 17, 2018
@betalb
Copy link
Author

betalb commented Nov 18, 2018

Yes, latest version before exporting of boundMethod and boundClass separately had this issue
Looks like previous commit introduced it 98968ee

Before this commit new function will just override bound function, but it won't be re-bound later on. Above comit tried to overcome this by deleting original descriptor and letting binding to happen again, but this time binding will be done on function that might be instance-specific, because we are working with instance, not prototype in this case.

In my case I was taking already bound function and debounced it

@stevemao
Copy link
Collaborator

@betalb Thanks for reporting this. PR welcome :)

betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
betalb added a commit to betalb/autobind-decorator that referenced this issue Nov 20, 2018
@betalb
Copy link
Author

betalb commented Nov 20, 2018

Created PR, but would like to revise it further, don't like appoach for super.method handling

@L-Yeiser

This comment has been minimized.

@stevemao

This comment has been minimized.

@victor-homyakov
Copy link

I'd like to add, that there is also a possibility of memory leak.

Minimal code to illustrate:

class Base extends React.Component {
  @boundMethod
  method() {}
}

class Child extends Base {
  method = debounce(super.method, 100);
}
  1. When constructing new Child(), the assignment method = debounce(super.method, 100) is executed
  2. The assignment method = debounce(super.method, 100) will get bound method, then debounce it, and finally will call setter
  3. The setter stores debounced bound method for all future instances.
  4. When constructing new Child() second time, the code method = debounce(super.method, 100) will receive bound debounced bound method, debounce it and store again.
  5. Each new instance will add its own level of bind + debounce and store the result for future instances.

bind and debounce consume memory themselves, but they also retain in memory their closures with all accessible variables.

Reproduction: https://codesandbox.io/s/autobind-debounce-memory-leak-jezks?file=/src/Child.js

What it looks like in DevTools:
memory leak

So, can you fix the bug and the memory leak in two years?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants