Y.throttle functions cannot be invoked within throttle-period of being created

[andrewnicols]

When defining a new Y.throttle, the initial late value is set to Y.Lang.now().
As a result, if you create a new throttled function, and it is immediately invoked before that time period has elapsed, it is ignored entirely.

Correcting this reveals that the unit tests are aware of this and expect it, but it is not a documented behaviour or, in my opinion, a desirable one.

There's even a unit test which checks that this is the case.

The attached patch changes the initial time to 0, and corrects the unit tests so that if a call is made before the throttle time has expired, it is still called.

[yahoocla]

CLA is valid!

[andrewnicols]

I realise that this change could affect people who have been working around this broken functionality for some time, but I still thing that it is the correct thing to do. I can't think of a reason that you'd want it to behave this way.

[caridy]

@andrewnicols, can you provide more details? I didn't understand the issue with the current implementation. AFAIK, Y.Lang.now() is save to use:

var a = Y.Lang.now();
var b = Y.Lang.now();
a < b

also, when you say "could affect people" can you elabore more? what will be the implication?

[andrewnicols]

Hi @caridy,

With the current implementation, if you create a throttled function, and use the default delay (for arguments sake - any will do) of 150ms then the test is actually b - a > delay. Expanded out that's:

Y.Lang.now - Y.Lang.now > 150

The function content will be skipped for the first 150ms after it is created. See https://github.com/yui/yui3/blob/master/src/yui-throttle/js/throttle.js#L44 for the code which does that.

We've seen this crop up a few times in the past when using the throttled event with event listeners. If you have an event which is only fired occasionally, but is fired within that initial throttle period, then those early-period fires will not be passed through to the function.

As an example, say you want something to happen on either domready, or on windowresize and you wish to throttle it (yes, I know that windowresize has it's own throttle already), you would expect the following to work:

// Throttle with the default delay of 150ms:
YUI().use('yui-throttle', 'event-resize', function(Y) {
  Y.on(['domready', 'windowresize'], Y.throttle(function() {
    console.log("Throttled event caught");
  }));
});

Disabling the throttle period (setting the delay to a value of -1) shows that the domready event is fired and everything is otherwise working. The domready event is just ignored because it's fired within that 150ms throttle period.

// Example with the throttle disabled (delay = -1ms) to prove that the event is actually working properly:
YUI().use('yui-throttle', 'event-resize', function(Y) {
  Y.on(['domready', 'windowresize'], Y.throttle(function() {
    console.log("Unthrottled event caught");
  }, -1));
});

Hope this helps,

Andrew

[andrewnicols]

Ah, sorry, I meant to also suggest possible implications.

Where people have worked around this issue in the past, fixing the issue may mean that they end up triggering the event twice.

In my example above, if someone has found that they're unable to throttle properly on that combination of events then they may have simply called the function outside of the throttle for the domready, as well as setting up the event listeners. Ideally they'd have taken the domready event out of the throttle condition too so it won't affect them, but if not they may have code resembling:

YUI().use('yui-throttle', 'event-resize', function(Y) {
  Y.on('domready', doStuff, this);
  Y.on(['domready', 'windowresize'], Y.throttle(Y.bind(doStuff, this)));
});

At present, they will only see a single call to doStuff(), but with this issue fixed, they'll see two calls to doStuff().

[caridy]

@andrewnicols will be providing better docs around the behavior, then we can merge this.

[andrewnicols]

To clarify the issue with an example:

var throttledFunction = Y.throttle(doSomething);
throttledFunction();

At present, the above example will not result in a call to doSomething.
Following this patch, it will be called.

[caridy]

LGTM, make sure you update the HISTORY for the component.