-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Drop unnecessary binding #12
Conversation
Correct |
I just traced a bug in my code to this change that dropped the unnecessary binding. I think it should be reconsidered and reverted because I'd argue it is necessary is some contexts. It is true that Consider: class Something {
private abc: string = '123'
fn() {
console.log(this.abc);
}
}
Something.prototype.fn = debounce(Something.prototype.fn, 100);
const b = new Something();
b.fn(); //An error will be thrown with this pull request's change because `this` is not `b` instance. In this case the debounced Would you agree? And would you consider a pull request that adds unit test along the lines of my example? |
* This reverts sindresorhus#12 * And it addresses same problem with pDebounce.promise() implementation added in sindresorhus#13
* This reverts sindresorhus#12 * And it addresses same problem with pDebounce.promise() implementation added in sindresorhus#13
* This is to protect against a mistake like sindresorhus/p-debounce#12 (comment) * Similar to sindresorhus/p-debounce#16, but for pThrottle.
* Add comments about importance of preserving `this` to avoid future mistakes like sindresorhus/p-debounce#12 (comment)
* This reverts sindresorhus#12 * And it addresses same problem with pDebounce.promise() implementation added in sindresorhus#13
this
here is alwayswindow
/global
, which means that if the functions actually usethis
, they will refer towindow
/global
… which is the default.Am I missing something?
Edit: I suppose this was just to apply the parameters before the spread operator became available.