-
Notifications
You must be signed in to change notification settings - Fork 108
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
[feature] Missing API: Scheduler#postTask #418
Comments
I think point two is done. At least I think I had something to do with it:
|
What is point one about? Like some example code (that we should mock) would help my feeble mind. |
|
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 |
ESM as well? But that's promising? 😃 |
Not having |
@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 😊 |
Hi @fatso83 and @benjamingr, I've been reading about The thing is,
let x = AbortSignal.timeout(10_000); but after 10 seconds it would become:
Plus, even if this timer was faked, how could we test it? It seems like we need a real Do you have any idea how I can approach this? I'd love some advice :) A small insignificant note: |
I'm not familiar with how sinon fake timers is implemented but when I need
to mock AbortController.timeout() I'll use
```typescript
timeout(n) {
const controller = new AbortController()
setTimeout(() => controller.abort(), n).unref()
return controller.signal()
}
```
Then I can use fake timers and timeout will work as expected.
Testing should not require fetch - you can assert that signal .aborted is
true after the timer expires
…On Sat, May 20, 2023, 12:54 PM Itay ***@***.***> wrote:
Hi @fatso83 <https://github.com/fatso83> and @benjamingr
<https://github.com/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: image]
<https://user-images.githubusercontent.com/37377389/239706213-1581b1b4-b432-4427-a8d2-fc01d2c142ca.png>
but after 10 seconds it would become:
[image: image]
<https://user-images.githubusercontent.com/37377389/239706757-97f8ab5b-6d45-4c91-a801-e409da2ea9a6.png>
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 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
—
Reply to this email directly, view it on GitHub
<#418 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6WM26LF4U77RA36MQZ3DXHEONRANCNFSM5NASE6KQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
@everett1992 I am not actually 100% convinced that a DOM API like Say we approach it like you proposed: 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? |
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. |
Since the first two points are done (timers/promises was added in #495), I'll just rename the issue to |
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?)None of these new APIs are particularly hard to mock but I haven't had a chance to look at them yet.
The text was updated successfully, but these errors were encountered: