remove needless statement

[lessfish]

!timeout must be true, so we can remove this needless statement

[jgonggrijp]

I agree that in theory, your change should not make a difference and that the removal of the statement would be desirable in that case. In practice, however, it causes the throttle re-entrant test to consistenly fail where it would otherwise consistently pass (at least on my machine under Node 10):

underscore/test/functions.js

Lines 406 to 429 in ca7d3f5

	QUnit.test('throttle re-entrant', function(assert) {
	assert.expect(2);
	var done = assert.async();
	var sequence = [
	['b1', 'b2'],
	['c1', 'c2']
	];
	var value = '';
	var throttledAppend;
	var append = function(arg){
	value += this + arg;
	var args = sequence.pop();
	if (args) {
	throttledAppend.call(args[0], args[1]);
	}
	};
	throttledAppend = _.throttle(append, 32);
	throttledAppend.call('a1', 'a2');
	assert.strictEqual(value, 'a1a2');
	_.delay(function(){
	assert.strictEqual(value, 'a1a2c1c2b1b2', 'append was throttled successfully');
	done();
	}, 100);
	});

  ✖ throttle re-entrant
    append was throttled successfully
    a1a2c1c2[object global]undefined != a1a2c1c2b1b2

I can't explain the effect, but given that the change seems to break something, I don't think this should be merged as-is.

FWIW, I made an attempt at simplifying _.throttle, too, but I couldn't do it in a way that kept all the tests passing.

[lessfish]

@jgonggrijp you are right, thanks

[kritollm]

This was interesting. I was going to suggest the same thing, but also to remove the reset of context and args. When I did, it passed all the tests.

I know why, and I think it's wrong to reset args and context as the last call to the debounced function should win and every call to debounced function updates them anyway.

This doesn't work:

result = func.apply(context, args);
// append returns here. Context is now 'b1', and args is 'b2' 
context = args = null; 
// cleared so the last call fails

Heres why the test fails:

1. throttledAppend is executed and calls append immediately.
2. append calls throttledAppend who store context and args, and set up a timer.
3. when timer ends, later (inside debounced) calls append. Append calls throttledAppend who store context and args and set up a timer.
4. When append return to the later function, context and args is cleared.
5. the last call to append is executed when timer ends, but context and args is cleared.

export default function throttle(func, wait, options) {
  var timeout, context, args, result;
  var previous = 0;
  if (!options) options = {};

  var later = function() {
    previous = options.leading === false ? 0 : now();
    timeout = null;
    result = func.apply(context, args);
    //if (!timeout) context = args = null;
    //context = args = null; // Fails if this is uncommented
  };

  var throttled = function() {
    var _now = now();
    if (!previous && options.leading === false) previous = _now;
    var remaining = wait - (_now - previous);
    context = this;
    args = arguments;
    if (remaining <= 0 /*|| remaining > wait*/) { // remaining > wait will never happen (I think)
      if (timeout) {
        clearTimeout(timeout);
        timeout = null;
      }
      previous = _now;
      result = func.apply(context, args);
      //if (!timeout) context = args = null;
    } else if (!timeout && options.trailing !== false) {
      timeout = setTimeout(later, remaining);
    }
    return result;
  };

  throttled.cancel = function() {
    clearTimeout(timeout);
    previous = 0;
    timeout = 0;
    //timeout = context = args = null;
  };

  return throttled;
}

[jgonggrijp]

@kritollm So you basically found that there is a race condition between the outer invocation of later and the inner invocation of throttled on the result = func.apply(context, args) line. Very clever!

This also explains why the throttle re-entrant test exists in the first place, and why the if (!timeout) part is there. !timeout will evaluate to false if throttled was recursively invoked on the previous line.

I disagree that the resets should be removed; they are there to prevent memory leaks. However, the race condition deserves some documentation. Would you like to submit a PR that adds a comment above the if (!timeout) line and get the credit for it?

[kritollm]

I agree that the reset of args and context is needed. Shouldn't throttled also use restarguments?

What do you think of this comment?

// This check is needed because the argument function can recursively invoke throttled

[jgonggrijp]

Shouldn't throttled also use restarguments?

Probably not strictly necessary, but if you like, you can try whether it makes for more compact minification (using npm run weight). If the difference is negligible, it's probably better to leave it as-is.

What do you think of this comment?

// This check is needed because the argument function can recursively invoke throttled

Pretty good. I'd write func instead of "argument function" in order to make it a bit more explicit, and add a little bit of Markdown markup for the annotated source.

// This check is needed because `func` might recursively invoke `throttled`.

If writing it myself, I might be inclined to write something like the following, in line with the overall literate style of the code:

// We use `timeout` to detect cases where `func` recursively invokes `throttled`.

But please do feel free to be a little bit creative here.

[kritollm]

Should we try to clarify whether the remaining > wait check is necessary?

if (remaining <= 0 || remaining > wait) {

As I see it, it will always be false, unless previous is greather than now(). From what I can see from the code, previous will always be less than or equal _now.

[jgonggrijp]

I suppose previous might be greater than _now if the user manually changes the system clock in between two calls to throttled. This would be most likely the case if the computer clock went out of sync with the GPS, for example if it had no power for a while.

As for whether it is better to timeout again in that case or invoke immediately, I find it hard to judge. But since a choice was already made, it's probably best to stick with it.

[kritollm]

You're right about that. There is a possibility that it can happen, and then the remaining can be much larger than wait. In the worst case, throttled might not trigger in several years.

So the check is actually very necessary (because the timeout will then be the value of remaining ).

} else if (!timeout && options.trailing !== false) {
      timeout = setTimeout(later, remaining); // 'remainig' can be much greather than' wait'
    }