sinonjs
Add more specific information in the migration guide to v19 about Fake Timers and toFake
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:
clock = useFakeTimers(now.toDate());
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:
clock = useFakeTimers({ now: now.toDate(), toFake: ['Date'] });
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.
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.~ (I do not have the time)
I think one way intuitive way (at least to me) to group timers is whether or not they depend on time passing to fire. This would mean one group of containing items that relate to real time such as setTimeout, setInterval, and Date and another group containing items that relate to the event loop such as nextTick and queueMicrotask.
I am not familiar with all the ways the fake timers package is being used but I believe one common desired behavior is to keep the event loop spinning as normal but for time to act as if it is frozen. The default behavior might then be to fake apis in the time group but not the event loop group.
However the event look group should arguably then include apis such as setImmediate, requestAnimationFrame, and requestIdleCallback. As I understand it, these apis were always faked in the past, even before the PR to have all timers be faked by default. This might be an argument against this grouping, or at least against only time group being enabled by default.
