handle negative infinity timeout #165
Conversation
| calls.push("-Infinity"); | ||
| }, Number.NEGATIVE_INFINITY); | ||
| this.clock.runAll(); | ||
| assert.equals(calls, ["NaN", "Infinity", "-Infinity"]); |
There was a problem hiding this comment.
without the code change, -Infinity is before Infinity in the array
src/lolex-src.js
Outdated
| @@ -196,6 +208,9 @@ function addTimer(clock, timer) { | |||
| timer.type = timer.immediate ? "Immediate" : "Timeout"; | |||
|
|
|||
| if (timer.hasOwnProperty("delay")) { | |||
| if (!isNumberFinite(timer.delay)) { | |||
There was a problem hiding this comment.
So I checked - the spec actually says (if < 0 then = 0) but Infinity isn't explicitly treated - >Int32 numbers overflow and also get set to 0 but not explicitly.
Node on the other hand sets the value to 1 (and not 0) https://github.com/nodejs/node/blob/master/lib/internal/timers.js#L19 done in https://github.com/nodejs/node/blob/master/lib/internal/timers.js#L55
There was a problem hiding this comment.
There was a problem hiding this comment.
(I've never read much spec, so all questions are in good faith)
From the linked issue in Jest, I read it as point 8 in https://heycam.github.io/webidl/#abstract-opdef-converttoint (as it's a long, not EnforceRange), namely
If x is NaN, +0, +∞, or −∞, then return +0.
There was a problem hiding this comment.
Oh, so we are in agreement. Cool! 😀
Do you think it's a bug in Node?
I suppose it's easy enough to emulate Node's behavior when we think we're faking timers in Node (as we have the addTimerReturnsObject variable)
There was a problem hiding this comment.
Seeing as the tag failed, I'll be a bit cheeky and do @bnoordhuis
There was a problem hiding this comment.
Do you happen to remember why timers are set to 1 and not 0?
Because that's what browsers did at the time. :-)
There was a problem hiding this comment.
So glad you guys are looking into the sweaty details of this. Delegation is 👑 Hope you add some comments to the implementation to make code archaeology easier for future maintainers :)
There was a problem hiding this comment.
I've opened an issue over at core about the bahvior - I think it's fine to be honest, it's not what the timers spec says but it's what Chrome and Node do basically nodejs/node#20596
There was a problem hiding this comment.
FWIW, I'm fine setting it to 1. That's not observable from my test - the test is the order in which the callbacks gets invoked, not if the timeout is 0 or 1.
Node and Chrome behaves the way I expect for Infinity, -Infinity and NaN.
setTimeout(() => console.log(1), Infinity);
setTimeout(() => console.log(2), NaN);
setTimeout(() => console.log(3), -Infinity);
// => 1
// => 2
// => 3While Lolex does 2, 3, 1. This PR fixes that, which is what matters to me 🙂
|
rebase to get merged :) |
|
Rebased! |
|
2.4.1 out. |
|
Awesome. Updated the PR in Jest. Getting closer! |
See this issue in Jest, with links to spec: jestjs/jest#5960. Without changing the implementation of Lolex, Jest test suite breaks when attempting to use it