Debounce bug #2093
Debounce bug #2093
Conversation
Now only uses 1 timer to handle both wait and maxWait. Code refactored to reduce the number of edge cases that require special logic fix issues with leading: false not always being honored
The test was testing that throttle(func, wait, {leading: false}) would return 2
results in a shorter time frame than the `wait`.
With a wait=128,
T:0 throttled func called
T:128 func should be called (trailing edge)
T:192 assertion tests func was called
T:192 throttled func called
T:254 assertion test func should not be called yet
T:288 erroneous assertion func should be called
T:320 func should be called (trailing edge)
T:384 assertion tests func was called
| throttled(); | ||
| }, 192); | ||
|
|
||
| setTimeout(function() { |
There was a problem hiding this comment.
Once I fixed the other bug, this test started failing. After reviewing the test, it seems like it had some bad assertions. Or maybe I am misunderstanding the way throttle works.
What do you think?
There was a problem hiding this comment.
I'm not sure how an added assert changes the behavior of a failing test.
Can you explain a bit more what the test change does?
There was a problem hiding this comment.
I changed the timings of the assertions. The commit message for this change has a bit more details: 43d71ef
|
@bman654 Thanks for this! Have you signed our CLA? |
|
yes (3/7/2016 15:07:59) |
|
🚀 ! |
| * jQuery(source).on('message', debounced); | ||
| * | ||
| * // Cancel the trailing debounced invocation. | ||
| * jQuery(window).on('popstate', debounced.cancel); |
There was a problem hiding this comment.
The debounce code can be a bit complex and understanding the ins and outs of its async-ness can be challenging at times. Would you mind walking me through the changes step by step (baby step it)?
There was a problem hiding this comment.
Sure. Let me add an additional commit that adds some more detailed comments. I could also walk you through the code in a screenshare.
There was a problem hiding this comment.
I could also walk you through the code in a screenshare.
😍
|
Thanks for the screenshare! |
|
I studied #1952 use case and the OP was describing a sustained period of activity and the I took his code and applied it to this new version (replaced his click handlers with setInterval). https://jsfiddle.net/7atuxyc3/ It looks like the new code is just as accurate. The throttle calls are occurring every 400ms +/- 8ms, which is all the accuracy you can hope for when using setTimeout. |
| remainingWait, | ||
| shouldInvoke; | ||
|
|
||
| if (waitTime >= wait) { |
There was a problem hiding this comment.
Is this condition for time zone switches?
There was a problem hiding this comment.
That bit is a little lower (line 9078)
|
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. |
3 participants
fixes #2089