sinonjs/fake-timers

New missing APIs

benjamingr opened this issue ยท 20 comments

There are a few things missing in fake-timers we need to add for Node 17 and for new browser APIs namely:

  • We're missing mocking of timers/promises and scheduler.wait (in timers/promises)
  • We're missing Symbol.toPrimitive on Node timers (right?)
  • We're missing scheduler.postTask with a delay

None of these new APIs are particularly hard to mock but I haven't had a chance to look at them yet.

I think point two is done. At least I think I had something to do with it:

$ ag -A10 toPrimitive
src/fake-timers-src.js
599:                [Symbol.toPrimitive]: function () {
600-                    return timer.id;
601-                },
602-            };
603-            return res;

What is point one about? Like some example code (that we should mock) would help my feeble mind.

import {setTimeout} from 'timers/promise'

not sure if it's possible to mock? unless it's also available somewhere globally so we can replace it

That was new. Thanks.

Seems posible @SimenB

โžœ  node git:(master) node
Welcome to Node.js v16.10.0.
Type ".help" for more information.
> let orig = require('timers/promises').setTimeout;
undefined
> require('timers/promises').setTimeout = function (fn) { fn(); }
[Function (anonymous)]
> require('timers/promises').setTimeout(() => console.log('foo'), 10000); // logs immediately
foo
undefined

(Of course, it should return a promise and probably just reuse the util.promisify version - this is just to show it's mockable)

ESM as well? But that's promising? ๐Ÿ˜ƒ

Not having timers/promises is becoming more and more of a nuance as Node.js 18 went to LTS October 2022, meaning more and more try to use it. Especially problematic in Jest where we now are resorted to using callback-based setTimeout() etc. when the other ones are nicer to read and use.

@thernstig Want to have a go? That's the best chance of finding something to scratch your itch. We accept PRs quite gladly ๐Ÿ‘

@fatso83 I try to help where I feel comfortable in the code but this is not such an area, otherwise I would already have pushed a PR.

What I wanted to highlight was that the new APIs have gone from experimental -> stable due to Node.js 18, possibly adding a new piece of information to this thread. I did not mean to sound pushy in a sense that I put any expectations on anyone to use their free time to fix it.

@itayperry wanna take a stab at it? I won't have time to take a look in the next ยฑ2 months probably due to travel/life.

Hi @benjamingr, It looks very interesting, and I'd like to try and help. I know you said it's supposed to be easy, but it seems hard.

It'd be great if you told me where to start ๐Ÿš€ what's the first thing I should mock? Maybe we can even break it into very small tasks :) and then I could do a few small PRs. In that case, if I get stuck someone else could continue from where I stopped (last issue I had took me too long cause it was a bit tricky).

I'll give it a shot โšก๏ธ

Also missing AbortSignal.timeout

@itayperry Would be great if you had a stab, and Ben already split these into smaller chunks. I would start with the bottom one in the list, then work my way up. AbortSignal.timeout is also a small and probably doable task that one could start with.

@fatso83 sounds great ๐Ÿ˜Š
I'll start with AbortSignal.timeout then :)

Hi @fatso83 and @benjamingr, I've been reading about AbortSignal.timeout() static method and I'm not sure how to fake it.

The thing is, AbortSignal.timeout() is very different from other timers:

  1. It does not return an id (but an AbortSignal), for example:
let x = AbortSignal.timeout(10_000);

would first be:
image

but after 10 seconds it would become:
image

  1. It does not have a callback, just a value of time (number).
  2. It cannot (or doesn't-need-to) be cleared.

Plus, even if this timer was faked, how could we test it? It seems like we need a real fetch that uses a real URL to test it.

Do you have any idea how I can approach this? I'd love some advice :)

A small insignificant note:
I tried using it with Google Chrome and it seems to respond with AbortError instead of TimeoutError, and there's an open bug about it: https://bugs.chromium.org/p/chromium/issues/detail?id=1431720&q=1431720&can=2

@everett1992 I am not actually 100% convinced that a DOM API like AbortController should be something we should target in fake-timers, but your solution seems like a possible approach, but it still leaves the issues @itayperry mentions on how this API should look like/work?

Say we approach it like you proposed: AbortSignal.timeout = timeout then we have the possibility to at least check if the timers/signal has been called by simply ticking and checking status flags:

const signal = AbortSignal.timeout(10000);
const fetchOp = fetch(url, {signal});
clock.tick(10000);
assert(signal.aborted);
clock.tickAsync(1);
try { await fetchOp }
catch (ex) {
  assert.instanceOf(AbortError)
}

Is this how you envision using it?

stale commented

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.

Seems like a fair issue to unstale.