fix throttle and debounce to be re-entrant

[ashanbrown]

It looks like debounce and throttle are not re-entrant because they clear the function context and args after executing the underlying function, even if the underlying function may call the original function itself. I assume this is done to allow garbage collection. I noticed this because I was triggering a debounced function in an ember application whenever I observed a change in a property that the function might itself change. Turns out ember has it's own throttle and debounce functions but I thought I'd offer up this patch anyway. I believe the fix is to only clear the context and args if a new timer has not been set up.

Andrew

[jdalton]

👍, I remember when Underscore removed the unit test for recursive throttle.

[jdalton]

Can this be simplified?
The context setting as part of the count seems like unnecessary complexity for the test.

[ashanbrown]

The context variable provides the value for 'this', which is actually the problem I saw in the first place ('this' being the ember object with a debounced observer). The problem is that context and args are lost, so I think that the test needs to set them.

I did think of that the test might be better if it used string appends rather than counters, but I initially was just following the form of the other tests. With string appends it might look like this:

asyncTest('throttle re-entrant', 2, function() {
    var value = '';
    var throttledAppend;
    var context = 'a';
    var arg = 'b';
    var count = 2;
    var append = function(arg){
      value += this + arg;
      if (count > 0) {
        count--;
        throttledAppend.apply(this, [arg]);
      }
    };
    throttledAppend = _.throttle(append, 32);
    throttledAppend.apply(context, [arg]);
    equal(value, 'ab');
    _.delay(function(){
      equal(value, 'ababab', 'incr was throttled successfully');
      start();
    }, 100);
  });

but I don't think it makes a big difference (same number of lines).

[jdalton]

The context variable provides the value for 'this', which is actually the problem I saw in the first place

I'm aware of this but was wondering if it could be simplified. I tackle this in my own implementation and have some tests on it. I'll review tonight and see if it can be refined.

That said the issue is valid and the fix aligns with what I've done.

[ashanbrown]

Maybe you were just pointing out that the variables were only used once? I just pushed a small simplification you can look for that. I think I originally had put them in variables because I wasn't sure how many times to call them. I ended up using them only once, which seemed sufficient to demonstrate the failure.

[jdalton]

Maybe you were just pointing out that the variables were only used once?

It's not that. The way the test is it's hard for me at a glance to see how many times _.throttle or _.debounce is expected to execute without juggling numbers in my head and trying to remember that context fits into that. It'd be great if you tracked contexts in an array of contexts, args in an array of args, and execution count separately then had asserts for each.

[ashanbrown]

Got it. Just pushed a modification that might make it clearer what the sequence of events is. It's kind of interesting that the expected sequence order needed to be flipped in the throttle re-entrant.

[saifelse]

@jashkenas could we get a a 1.6.1 release for this? It should be backwards compatible, as no one should expect their debounced functions to be called with the wrong context.

[mponizil]

Thanks for this fix!