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

Fix trailing edge issue in throttle #3053

Closed
wants to merge 2 commits into from
Closed

Conversation

cpnota
Copy link

@cpnota cpnota commented Mar 11, 2017

PR to address issue #3051

Note: Given the state of flux of the repo, I was unable to run any scripts or tests, except for some local tests. There are also not any tests here, since they don't seem to be in this repo? Please advise.

Eventually I determined that the issue boils down to a fundamental incompatibility between the expectations for debounce and the expectations for throttle. Following the trailing edge, debounce does not (and should not) care about the the amount of time elapsed before the next leading edge. throttle does. There seem to be three possible approaches to resolving this:

  1. Decouple the throttle and debounce implementations again. No thanks.
  2. Reclaim maxWait in the name of House Throttle. In the original debounce-based throttle implementation, setting maxWait reset the lastCall time to the time that the trailing edge created by maxWait was invoked (see here. This is ostensibly not correct, which is probably why the behavior was changed at some point.
  3. Add a new option that throttle can utilize. This is the approach I went with.

I added an option called cooldown which lets you specify the minimum amount of time that must elapse between a trailing edge and a leading edge. Calls to debounced that occur in that interval are deferred until the end of that cooldown period (effectively creating a new trailing edge).

@jsf-clabot
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@cpnota
Copy link
Author

cpnota commented Mar 13, 2017

btw I signed the CLA, but the check hasn't updated. @jsf-clabot ?

@jdalton
Copy link
Member

jdalton commented Mar 13, 2017

@cpnota The CLA should be signed with the name and email associated with your git config. You can update your git config info then git commit --amend --reset-author on your commits and force push.

@jdalton
Copy link
Member

jdalton commented Mar 18, 2017

Closing as abandoned by OP.

@lock
Copy link

lock bot commented Dec 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants