Closed SimenB closed 2 months ago
Merging #323 into master will decrease coverage by
0.03%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #323 +/- ##
==========================================
- Coverage 92.75% 92.71% -0.04%
==========================================
Files 1 1
Lines 552 549 -3
==========================================
- Hits 512 509 -3
Misses 40 40
Flag | Coverage Ξ | |
---|---|---|
#unit | 92.71% <100.00%> (-0.04%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Ξ | |
---|---|---|
src/fake-timers-src.js | 92.71% <100.00%> (-0.04%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Ξ = absolute <relative> (impact)
,ΓΈ = not affected
,? = missing data
Powered by Codecov. Last update 7f8322e...21d726f. Read the comment docs.
So here's the thing: promise libraries and other libraries use nextTick and microticks for scheduling so this is a pretty big breaking change.
In principle I agree that we should mock this by default.
So here's the thing: promise libraries and other libraries use nextTick and microticks for scheduling so this is a pretty big breaking change.
Good point, should probably call it out in the readme.
In principle I agree that we should mock this by default.
π
@benjamingr added a section to the readme, feedback appreciated π
@SimenB I am +1 on this change in principle but I know it will break pretty much every fake-timers install "in the wild" that uses the current API.
Should work if people use the builtin Promise
and async-await
, no? If it breaks builtin Promise
global I'd consider that a bug in node.
My main issue is that to enable "fake everything" I have to enumerate all timers from the outside, which is not obvious I think. Maybe we could rather add a fakeNextTick
explicit option (defaulting to false
I guess) instead of piggy-backing toFake
?
Should work if people use the builtin Promise and async-await, no? If it breaks builtin Promise global I'd consider that a bug in node.
It doesn't break the built in promises but mostly because we don't mock its timings at the moment (we probably should with async_hooks). In practice this means that it's a lot harder than it has to to test promises with fake-timers (I recall a lot of discussions about this + async versions of timers somewhere?)
I don't mind seeing what breaks with mocking nextTick on some large codebases I maintain - though I assume I know the answer is "every place that relies on nextTick executing and doesn't .tick or runMicrotasks before that :]
As a data point, in a work project where I migrated to use Jest's fake timers now that Jest ships with Lolex it broke node-fetch
(and/or integration with nock
, not entirely sure, I just added a runMicrotasks
call)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Stale bot?!
This makes my head hurt and you guys know best anyhow. Not sure why it has stalled, though?
B,B,B,B,bump? It's not super problematic to release a major version, as long as you also document how to deal with the fallout. Like using node-fetch as an example case, show what needs to be done to fix the test cases. @SimenB
This needs a rebase to resolve the conflicts.
@SimenB do you have time to take this across the finish line? If not, would you be ok with someone else doing it?
I want to finish this (and do #490) and release a new breaking major. Soonish
I don't mind seeing what breaks with mocking nextTick on some large codebases I maintain
There's been 4 years. I guess the number of places that rely on non-native Promises are a lot fewer and further between. Maybe it's not that bad? I think we would just need to add something to the changelog.
Since I closed #490 and I haven't heard anything in months, I think I'll just pick this up and release a new breaking major.
Weird how much you can get done in a weekend without kids π
Purpose (TL;DR) - mandatory
It's very weird to me that this method is excluded from default mocking. I've read #126 and to me the solution seems to be for users to specify a
toFake
themselves, not force other people to specify "yes, when I say fake timers I do mean all timers".Note that this should be considered a breaking change.
Whenever we start supporting some new timer, it should by default not be mocked, but I think when we later make a new major release it should be mocked by default again.