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

Incorrect throttle definition #33

Open
Wykks opened this issue Jul 8, 2015 · 7 comments
Open

Incorrect throttle definition #33

Wykks opened this issue Jul 8, 2015 · 7 comments

Comments

@Wykks
Copy link

Wykks commented Jul 8, 2015

In the test "it will execute every threshold ms", the result expected is [0, 11, 22, 33, 44, 55]
But If we strictly respect the definition of throttle (and the name of the test) :
throttle is used to make sure the function doesn't execute more than once _per_ x milliseconds
It should be [0, 10, 20, 30, 40, 50] right ?

Also, when I run the solution with

var interval = setInterval(throttled, 5);

I get [0,16,28,40,52] ; but it should be [0,10,20,..] ?
setTimeout should not take ms but the difference between the last call, in order to call the func as soon as possible.

How I see throttle according to the definition and the test "it will execute at least once more to make up for swallowed calls" :
With an interval of 6ms and 10ms for throttle (EDIT: Updated with 6ms)
Call 1 (0 ms) direct call ==> [0]
+6ms
Call 2 (6 ms) --> timed for 4s (not 10ms!) ==> [0]
+4ms @10ms Call 2 end ==> [0, 10]
+2ms
Call 3 (12 ms) --> timed for 8s (not 10ms!) ==> [0, 10]
+6ms
Call 4 (18 ms) --> clear Call 3 ; timed for 2s ==> [0, 10]
+2ms @20ms Call 4 end ==> [0, 10, 20]
+4ms
....
I know that doing this is not very relevant (at least with such low interval).
Maybe I'm just over-thinking the problem, but the definition (or the swallowed calls test) should be more explicit about this behavior.

EDIT 2 : Without this swallowed calls test weirdness, it would be even more simple. And we would get [0,12, 24, 36, 38] as 2 * 6ms > 10ms ; 1 call skipped each time. And it strictly respect the simple need of "limit to one call per x ms".

Thanks for this btw !

@darrenscerri
Copy link

I agree, my solution returns [ 0, 10, 20, 30, 40, 50 ] which as per my understanding would by acceptable with respect to the throttle definition that "it will execute every threshold ms". To allow the tests to pass I'm adding 1ms to the ms argument passed to the throttle function.

@chemzqm
Copy link

chemzqm commented Oct 22, 2015

My result changed a lot
[ 0, 11, 22, 32, 43, 54 ]
[ 0, 12, 22, 32, 42, 52 ]
[ 0, 12, 21, 32, 43, 54 ]

@PinkyJie
Copy link

I agree, the correct answer should be [ 0, 10, 20, 30, 40, 50 ].
In addition, I think there are 2 more incorrect test cases.

  1. throttle will execute at least once more to make up for swallowed calls:
    • I think the correct answer should be 1.
  2. throttle gets called with the later arguments:
    • I think the correct answer should be [ 11, 22, 33 ]

This article has a fancy animation to illustrate the difference between throttle and debounce.

@kolodny
Copy link
Owner

kolodny commented Oct 26, 2016

I never really gave throttle the proper attention. My first pass at it had the intervals being kicked back into place, for example 10ms kicks at (1,41,63,95,149) => (0, 40, 60, 90, 140), and didn't use mock timers at all. I want to reexamine that approach and also take a closer look at the pending PRs for throttle

@mtso
Copy link

mtso commented Feb 24, 2017

I am also getting [ 0, 10, 20, 30, 40, 50 ] with sinon's fake timer.

@PinkyJie The rest of the test cases appear to be accurate, given that:

  1. Calls made to throttle during the throttle "cool down" should be collapsed into a single call made at the end of the throttle, so calling the throttled thunk twice within the threshold should make it call once at the beginning, and then once at the end. The thunk should thus be called twice.
  2. The [33, 44, 55] assigned into args is the result of the above test case behavior.

@kolodny
Copy link
Owner

kolodny commented Feb 28, 2017

This is tricky to fix with fake timers, I've switched to use a "kicker" instead. Let me know if it solves this:

https://github.com/kolodny/exercises/tree/fix-time-issues

@sunderls
Copy link

met the same problem,

just a mention that lodash.throttle generates
[0,10,20,30,40,50] so I'd say we should stick to lodash

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

No branches or pull requests

7 participants