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

Suggestion: API parity with clearTimeout and clearInterval #4

Closed
JamesMGreene opened this issue Oct 19, 2017 · 4 comments · Fixed by #9
Closed

Suggestion: API parity with clearTimeout and clearInterval #4

JamesMGreene opened this issue Oct 19, 2017 · 4 comments · Fixed by #9

Comments

@JamesMGreene
Copy link
Contributor

JamesMGreene commented Oct 19, 2017

What are your thoughts on completing API parity with the core Timers APIs by adding the top-level methods clearTimeout and clearInterval to this module's API?

Obviously it would be simple to do so:

exports.clearTimeout = function (timer) {
	if (timer instanceof Timeout) {
		timer.clear();
	}
};

exports.clearInterval = function (timer) {
	if (timer instanceof Interval) {
		timer.clear();
	}
};

Happy to submit a PR for this, plus unit tests, if you're good with the suggestion.

@JamesMGreene
Copy link
Contributor Author

I'd also really like to see these added as I am going to recommend that known consumers of my older module safetimeout (basically the same intent as your module) switch over to safe-timers.

@JamesMGreene
Copy link
Contributor Author

@ronkorving: Thoughts on this one?

@ronkorving
Copy link
Collaborator

Sure, sounds good 👍
I think you don't even really need the if-statements by the way.

This also works in the browser:

var t = setTimeout(function () { console.log('fired'); }, 10);
clearInterval(t);

t does not fire.

@JamesMGreene
Copy link
Contributor Author

JamesMGreene commented Oct 23, 2017

@ronkorving: Well, the browsers' behavior is really just an implementation shortcut/quirk rather than a guaranteed spec-compliant behavior. Regardless, if you are suggesting to keep this module's implementation similarly loose (which it seems like you are), then I'm fine with that.

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 a pull request may close this issue.

2 participants