This repository has been archived by the owner on Jul 30, 2018. It is now read-only.
Guarantee util.throttle delay #360
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ack will fire is less amount of time. Modify util.throttle and util.debounce so the delay is always guaranteed.
bryanforbes
previously requested changes
Oct 30, 2017
tests/unit/util.ts
Outdated
| let timer: Handle | null; | ||
|
|
||
| return { | ||
| afterEach() { |
There was a problem hiding this comment.
Is this (and the ones below) needed with the suite-level afterEach()?
src/util.ts
Outdated
| if (delay == null || delta >= delay) { | ||
| callback(); | ||
| } else { | ||
| timerId = setTimeout(timeoutHandler); |
There was a problem hiding this comment.
Should this pass delay - delta as the second argument to setTimeout()?
There was a problem hiding this comment.
Yes, I goofed that when I changed it to make delay optional. I'll fix it.
|
The test failure "Attempt to detect unregistered has feature "web-worker-xhr-upload" is addressed in another ticket/PR: PR #361 |
edhager
dismissed
bryanforbes’s stale review
All of Bryan's suggestions have been implemented.
kitsonk
approved these changes
Nov 3, 2017
maier49
reviewed
Nov 3, 2017
src/util.ts
Outdated
| } else { | ||
| // Cast setTimeout return value to fix TypeScript parsing bug. Without it, | ||
| // it thinks we are using the Node version of setTimeout. | ||
| timerId = <any> setTimeout(timeoutHandler, delta); |
There was a problem hiding this comment.
Should we be timing out for delay - delta? What we're saying here is that we want to wait as long as we've waited right? That seems arbitrary
maier49
approved these changes
Nov 3, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
setTimeout in Edge and IE 11 do not guarantee the delay value. In my testing, if the delay is greater than 200ms, then setTimeout is reliable but with a smaller delay, the callback function can fire early roughly 50% of the time.
I created
util.guaranteeMinimumTimeout()which makes sure the desired amount of time has passed.Fixes: #357