Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more specific information in the migration guide to v19 about Fake Timers and toFake #2620

Open
neverbot opened this issue Oct 1, 2024 · 2 comments

Comments

@neverbot
Copy link

neverbot commented Oct 1, 2024

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.

@fatso83
Copy link
Contributor

fatso83 commented Oct 1, 2024

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?

@fatso83
Copy link
Contributor

fatso83 commented Nov 7, 2024

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 sinonjs/fake-timers#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants