timers: fix validation #54404
timers: fix validation #54404
Conversation
nodejs-github-bot
added
needs-ci
github-actions
bot
removed
the
request-ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54404 +/- ##
==========================================
- Coverage 87.34% 87.33% -0.02%
==========================================
Files 649 649
Lines 182524 182614 +90
Branches 35026 35036 +10
==========================================
+ Hits 159420 159478 +58
- Misses 16373 16403 +30
- Partials 6731 6733 +2
|
|
I like the idea but maybe doing this is an argument to introduce |
|
@benjamingr That's different, IMHO. The scheduler API is experimental and thus might go away at anytime.
The number of letters doesn't really matter (thanks autocompletion ;)) but I'd like to give users a simple API they can use without the risk of overriding a well known global and therefore having side effects. |
|
@ShogunPanda we currently have 4 "official" ways to set timeouts (setTimeout, promises setTimeout, scheduler.postTask and scheduler.wait) as well as lots of others for specific timeouts (e.g. AbortSignal.timeout) . That's... already too many ways to do the same thing anyway, I doubt adding another would improve things. If there is a consensus I'll remove my block.
So is any new API we add like this one? |
|
I agree: let's wait for other consensus and can solve this. |
ShogunPanda
added
the
tsc-agenda
|
@benjamingr @jasnell @legendecas As agreed in TSC/1608 I started working on the warning for I then noticed that it was not the documentation that was lacking but rather the validation. Also, I noticed that documentation for At the end of the day, since I just refactored internal parts ( |
ShogunPanda
added
request-ci
github-actions
bot
removed
the
request-ci
|
Kinda, yes |
ShogunPanda
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 6d654dd |
| Calling `timersPromises.scheduler.wait(delay, options)` is equivalent | ||
| to calling `timersPromises.setTimeout(delay, undefined, options)`. |
There was a problem hiding this comment.
Because the documentation was incorrect (or not up-to-date). It was not roughly equivalent, it was strictly equivalent since it just wrapping it. I made that explicit.
There was a problem hiding this comment.
Then I guess we should document ref as a valid option
| } catch (err) { | ||
| return PromiseReject(err); | ||
| } |
There was a problem hiding this comment.
Why catch the error in the first place? Isn't returning a promise rejection strictly equivalent in an async function?
There was a problem hiding this comment.
Yes, I copied and pasted from setTimeout (which is not async) and missed that. Do you think we can leave like that or do we need a tiny fixing PR?
There was a problem hiding this comment.
It would have been more "clean" to close that PR about adding an alias for setTimeout, and open a new one for the validation changes. That way, folks watching the repo would have had more chance to notice the change, and our wait time policy would have give enough time for folks to chime in (all the changes happen during a weekend, making it even more likely fewer folks had time to have a look). All in all, I don't really care if you do the follow-up PR with my suggestion, but IMO it's the "right" thing to do.
|
I feel this landed prematurely. Although I gave approval I believe having at least an extra one would have been good. |
PR-URL: nodejs#54404 Reviewed-By: Claudio Wunder <cwunder@gnome.org>
PR-URL: #54404 Reviewed-By: Claudio Wunder <cwunder@gnome.org>
PR-URL: #54404 Reviewed-By: Claudio Wunder <cwunder@gnome.org>
PR-URL: #54404 Reviewed-By: Claudio Wunder <cwunder@gnome.org>
PR-URL: #54404 Reviewed-By: Claudio Wunder <cwunder@gnome.org>
This PR improves
timers/promisesin the following way:timers.set*now use the same validation logic. All arguments, including thedelayargument (where appropriate) are now validated.timers.scheduler.waitnow supports theoptions.refoption.timers.scheduler.wait. It was not just "roughly" equivalent totimers.setTimeout, it was just a wrapper around it. I updated the docs.