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

Fix: util.throttle misbehavior #111

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

ml1nk
Copy link

@ml1nk ml1nk commented Nov 11, 2017

In my opinion, the current implementation of $.fn.dataTable.util.throttle shows unexpected behavior if the timing is unfavorable:

  1. The time between two consecutive fn calls should be equal or greater than the given frequency
  2. fn should always be called with scope and arguments of the last tfn (throttled function) call

An example of this misbehavior is at the following link:
https://repl.it/Nq7e/48

I acknowledge that my contribution is offered under the MIT license.

@DataTables
Copy link
Collaborator

Awesome - thanks for this! I'm not going to merge this in to 1.10 if that's okay as I've started a 2.0 branch now and plan to do some work on the throttle there (also introducing a debounce option).

Looking at the code, that is an improvement on mine - thanks. It does however have the same issue as mine in that it is calling the callback immediately when triggered and then again after the timeout period. That is one of the things I plan to address. I'll do so in 2.0.

Thanks again.

@ml1nk
Copy link
Author

ml1nk commented Nov 11, 2017

I'm looking forward to see 2.0 as this project is already a very good one.

it is calling the callback immediately when triggered and then again after the timeout period

An immediate callback is probably optimal in most cases, because the additional setTimout is just overhead if there isn't a explicit reason to do so, for example if throttling events. In addition code that depends on this behavior breaks, like detection if the throttled function wasn't called recently before. At least there should be some kind of switch for this.

It could be something like the following:

function throttle(fn, freq, delayed) {
		var
			frequency = freq !== undefined ? freq : 200,
			call,
			that,
			timer;

		function timeout(direct) {
			if(direct && delayed) {
		   		setTimeout( function () {
		    			fn.apply(that, call);
				}, 0 );
			} else {
				fn.apply(that, call);
		 	}
			call = undefined;
			that = undefined;
			timer = setTimeout(function () {
				timer = undefined;
				if (call) {
					timeout(false);
				}
			}, frequency);
		}

		return function () {
			that = this;
			call = arguments;
			if (!timer) {
				timeout(true);
			}
		};
}

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

Successfully merging this pull request may close these issues.

2 participants