Implement node Timer api when running in node environment.#4622
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4622 +/- ##
==========================================
+ Coverage 56.17% 56.37% +0.19%
==========================================
Files 194 194
Lines 6544 6551 +7
Branches 3 3
==========================================
+ Hits 3676 3693 +17
+ Misses 2867 2857 -10
Partials 1 1
Continue to review full report at Codecov.
|
SimenB
left a comment
SimenB
left a comment
There was a problem hiding this comment.
LGTM. Only thing I'd change is to not overload the FakteTimers constructor and instead use an object, but no biggie
| refToId: (ref: number) => ref, | ||
| }; | ||
|
|
||
| this.fakeTimers = new FakeTimers( |
There was a problem hiding this comment.
Can we make the constructor take an object instead of multiple arguments?
There was a problem hiding this comment.
Good point! Updated.
| const timer2 = env1.global.setInterval(() => {}, 0); | ||
|
|
||
| [timer1, timer2].forEach(timer => { | ||
| expect(timer.id).not.toBe(undefined); |
|
CI has finished succesfully, not sure why it's still reported as in progress here. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
5 participants
Fixes #4559. Not entirely sure if this is the right approach. Let me know if there's a better way!
Some thoughts:
we might want to assert if ref/unref are really functions but I noticed this isn't always done in other places in the codebaseI had to shuffle the FakeTimer constructor args a little bit since some are optional. I considered making timerConfig an optional arg with a default implementation but given that FakeTimer is generic about TimerRef, I couldn't get Flow to typecheck it properly.Any feedback is more then welcome.
Thanks!