lib: move queueMicrotask to stable#25594
Uh oh!
There was an error while loading. Please reload this page.
Conversation
devsnekforce-pushed the queuemicrotask-stable branch 2 times, most recently from 49f907d to 156cd59Compare
mcollina commented Jan 20, 2019
Is this compatible with the other implementation? If so, have we got tests in wpt? |
devsnek commented Jan 20, 2019
Looks like this exists: https://github.com/web-platform-tests/wpt/tree/master/html/webappapis/microtask-queuing @joyeecheung how should i go about integrating the tests here? |
@devsnek The only test in the WPT that can be run in Node without modification is https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/microtask-queuing/queue-microtask.any.js (the other two either needs |
Trott commented Jan 21, 2019
Is test/known_issues/test-vm-timeout-escape-queuemicrotask.js a |
addaleax commented Jan 21, 2019
@Trott It’s a In any case, yes, it’s definitely not a |
sam-github commented Jan 21, 2019
I'm not sure if this is the right place, but I would like to suggest that the documentation be enhanced to describe why Node.js has this API, and/or under what conditions we think it should be used. https://nodejs.org/api/globals.html#globals_queuemicrotask_callback is ATM a purely factual description of what it does, but given the difficulty in knowing when to use I'm personally confused, because while the docs don't say anything on expected usage, there is some example code, that is virtually identical in purpose to the examples in process.nextTick, but with the additional mention of something about interaction with promises. The example just makes me wonder if all usages of nextTick somehow don't interact with promises, or perhaps the example is not specific enough. See https://nodejs.org/api/process.html#process_process_nexttick_callback_args to compare the examples, and description. For me, this kind of vagueness is fine for experimental APIs, but once they have had some use, it should be clarified. Its possible the docs should be something like:
If the above guess at the purpose is somewhat correct, the example should not be so similar to the nextTick example, but instead be a Promise.waitAll() implementation, or something of the like. |
devsnek commented Jan 21, 2019
It originally said something along the lines of "This API schedules a callback interleaved in the microtask queue, interleaved with promsies. Most Node.js code should use this API for scheduling. If you want to guarantee your callback runs before promises, use process.nextTick" but iirc @bnoordhuis removed it? |
sam-github commented Jan 21, 2019
If this is true, then the process.nextTick docs need to be modified to reflect that. As it is, we seem to have two different APIs that "should" be used for the same purpose, quite confusing. |
mcollina commented Jan 21, 2019
I disagree. Using this API triggers a costly JS <-> C++ transition, while |
devsnek commented Jan 21, 2019
@mcollina if performance is a concern we can definitely work on that, but nextTick jumps out of line and the zalgo of that isn't usually needed. |
sam-github commented Jan 21, 2019
@devsnek There are two lines. It would be equally true to say promises jump out of line. Since the nextTick line is used extensively inside node.js (not to mention the wider set of node.js user code), encouraging people to use a different queue seems more likely to create problems than to create consistency. Note for those not familiar: zalgo. The core of the problem is that promises created a new queue, and now there are two. I assume having the utask queue be integrated with the nexttick queue wasn't technically possible? I had the impression that v8 went through some effort to give embedders control over utask scheduling. And how does setImmediate fit into this? Didn't it come from the browser? Did the promises queue unleash zalgo in pure browser javascript, or did browsers retarget setImmediate at the utask queue? |
jasnell commented Jan 21, 2019
I'm -1 on any language that suggests that this be favored over nextTick for a number of reasons that have already been discussed here. We should say only what the differences between them are without placing any relative value or preference. |
joyeecheung commented Jan 21, 2019
I opened #25616 to run the WPT in core - to my surprise no additional modification to the current tooling is necessary to add this into core |
sam-github commented Jan 21, 2019
I'm a little uncomfortable with not specifying a preference, I think our API users deserve to be informed not only about how the APIs work, but how we expect them to be used. But I can see it is contentious. Perhaps we can at least acknowlege the tradeoffs between nextTick and and queueMicrotask, either in the API docs here, or we could link to the guide, and enhance it with information about the new queue. And despite the title of that guide, I notice it isn't linked to from the nextTick docs. I'll add it. |
Trott added the semver-minor 
lib/internal/bootstrap/node.js Outdated
There was a problem hiding this comment.
We can do
global.queueMicrotask=queueMicrotask;now no?
There was a problem hiding this comment.
we can, but it's best to be explicit.
There was a problem hiding this comment.
We do it for setTimeout, setImmediate, etc. I'm fine with it as is though.
There was a problem hiding this comment.
Making it global means semver-major.
There was a problem hiding this comment.
Ah! Right lol... Had forgotten that :)
There was a problem hiding this comment.
I personally prefer
global.queueMicrotask=queueMicrotask;It is significantly shorter and the implications should be clear.
lib/internal/bootstrap/node.js Outdated
There was a problem hiding this comment.
I personally prefer
global.queueMicrotask=queueMicrotask;It is significantly shorter and the implications should be clear.
BridgeAR commented Mar 6, 2019
@sam-github I agree that the docs could and probably should be improved further but to me it seems that can be done in a separate PR as this should mainly just move the queueMicrtask to stable without changing the documentation. @devsnek this needs a rebase. |
sam-github commented Mar 6, 2019
I think its odd to move something from experimental when we don't have consensus on what purpose it has, but its stable in the sense of "not going to change", so sure, +0. |
devsnek commented Mar 6, 2019
the two points were parity with browsers and providing an api that lets people enter the proper async queue. i feel like this api should be explained as "if you don't know why you need nextTick, use queueMicrotask." "why do i need nextTick" is probably legacy reasons or performance (we can get rid of the perf issue entirely by using |
sam-github commented Mar 6, 2019
Unfortunately, there is also a sizable "if you don't know why you need queueMicrotask, use nextTick" camp. And also a |
no one has yet given a reason why new code should use nextTick over queueMicrotask besides performance. I understand there are legacy timing reasons to use nextTick, but that seems to fall squarely in the category of "I know why I need nextTick" some reasons to use queueMicrotask:
|
BridgeAR commented Mar 6, 2019
This breaks master now because the test-bootstrap-modules limit is reached (probably not yet 2 days ago) |
PR-URL: nodejs#26520Fixes: nodejs#26528 Refs: nodejs#25594 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26520Fixes: nodejs#26528 Refs: nodejs#25594 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #26520Fixes: #26528 Refs: #25594 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
mcollina commented Mar 14, 2019
This breaks readable-stream test suite. Please don't include it in a release as it would need to be addressed or reverted. |
devsnek commented Mar 14, 2019
@mcollina can you provide some more info on that? |
mcollina commented Mar 14, 2019
The tests of readable-stream all comes out red. It might be something in the harness. |
targos commented Mar 14, 2019
What I saw in citgm is that the presence of the new global makes the tests throw an error |
devsnek commented Mar 14, 2019
is there some common test framework or something that is failing? if something needs to be updated we should make sure it is updated. I added queueMicrotask to the globals lib a while ago (sindresorhus/globals#142) |
devsnek commented Mar 14, 2019
seems like it just needs to be modified to collect globals at startup instead of using a known list cc @mcollina |
That file looks like a port of our |
mcollina commented Mar 15, 2019
Yes. However the switch of the enumerable flag makes this change semver-major in nature as it’s going to broke some module CI systems. I think it would be safer if this is backported to 11 with the enumerable flag disabled. |
12 participants
IIRC the critera for stable was better support from other platforms, which has since been fulfilled by safari and chrome.
Closes#25592
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes