Open neverbot opened 1 month ago
I guess this is the classic case of "you're screwed if you do, and screwed if you don't" 😄
I was more or less expecting the fallout of this change to result in stuff like this, which is why nextTick
and queueMicroTask
were previously skipped. Similarly, stubbing setImmediate
could (as you saw in the blog post) have adverse effects on some libraries that did not cache the references to these globals. The fix in code using globals that is not to be affected is to store a reference to the global before it is being replaced, instead of directly referencing the global. Of course, there might be loads of cases where that is unfeasible (such as being an end user of a 3rd party Promise library) and then you need to explicitly specify toFake
.
I think having to specify a long list of timers (many of which one might not even know of, such as the performance timers) is a bit much to ask of users, for what I assume is a quite common task. Could we perhaps improve the UX from the developer side by providing an easier escape hatch like one of these:
Suggestion A: ignore option
add a toNotFake: [ 'nextTick', 'setImmediate']
option. Would make it easier to just modify the defaults. Would of course need to be exclusive to toFake
Suggestion B: provide pre-defined constants of timer lists
export const ES_STANDARD_TIMERS = ["setTimeout", "clearTimeout", "setImmediate", "clearImmediate","setInterval", "clearInterval", "Date"]
export const WEB_TIMERS = [ "requestAnimationFrame", "cancelAnimationFrame", "requestIdleCallback", "cancelIdleCallback", "hrtime", "performance"]
etc
@SimenB ideas?
Another option: we could revert this change
Meaning we skipped nextTick
and setImmediate
to be friendlier for the most common case. The underlying library, fake-timers
, seems to be mostly used as a dependency of others, with Jest and Sinon being the two most prominent.
This whole change was essentially requested by the Jest community, and ironically, the version used in stable Jest (v29) is fake-timers@10 and in the alpha version it's fake-timers@11, so it's really just Sinon exposing it 3 months after release.
3 months on, the fallout has been mostly negative (like this issue and https://github.com/sinonjs/fake-timers/issues/507), and I am questioning whether or not this change really has much positive merit. While we could expose constants like I suggested above, maybe it would be better to do the reverse: revert to the previous behavior and expose constants for those in need of something different: ALL_TIMERS
, STANDARD_TIMERS
, etc
I'll hack together a proposal.
Is your feature request related to a problem? Please describe.
In https://github.com/sinonjs/sinon/tree/main/docs/guides/migration-guide.md the fact that there is a breaking change with the new version of Fake Timers is mentioned (and explained, but on a first reading I didn't thought our problem was related). I think the explanation could be improved with some examples.
Describe the solution you'd like
If you are testing Express endpoints and initialize the fake timer the old way:
every test will end with a not very descriptive timeout. I think it could be a pretty common situation, so the guide could mention that a possible common fix for those cases would be to fake just the Date object:
I found the hint to this in a blog post from 2018, but in our case it hasn't manifested itself until we upgrade to sinon v19. Both the blog post author and us had to debug the internals of express to understand what was happening.