-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
implement queueMicrotask #22951
implement queueMicrotask #22951
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change looks fine. Needs documentation added that preferably includes an explanation of how/why to use this.
lib/internal/bootstrap/node.js
Outdated
} | ||
}); | ||
}; | ||
Object.defineProperty(global, 'queueMicrotask', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new global is inherently semver-major. Adding this to util
first would allow it to go in as semver-minor (which is the same pattern we've used for introducing other bits e.g URL
, TextEncoder
, TextDecoder
, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with semver-major.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure that it is inherently a semver-major in node; what code would be broken by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding globals is semver-major as it changes the behavior of non-strict code :/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only non-strict code that's omitting var
/let
/const
and using this variable name, though - how likely is that combination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The policy isn't about likely, it's about possible. A case can be made to the @nodejs/tsc to request a downgrade to semver-minor.
semver-major as this adds a new global |
One significant discussion point on this that should be addressed both here and in any accompanying documentation is: Why would someone use this method as opposed to |
Related intent to ship in Chromium: https://groups.google.com/a/chromium.org/d/msg/blink-dev/LGCEO53Mb6A/9oc4olniBQAJ This is not documented in MDN yet, the only existing extensive documentation seem to be the spec itself (which is not normative i.e. no pressure to follow this spec). |
I'm general you should always use queueMicrotask. only use nextTick if you for some weird reason need to jump ahead of the microtask queue. I believe @bmeck at some point gave the example of throwing an error in the next tick but before microtasks run. |
What browsers support this? A quick check indicates that none of Chrome, Safari, and Firefox supports this yet, at least not as a global. Nor can I find it mentioned on a MDN page. Whereas TC39 has a well defined process with stages of proposals, the WhatWG has no such process. If this is not widely supported by browsers, I would suggest that the feature be marked as experimental as the spec may change as feedback from browsers is received. |
If this is not widely available in browsers, could you attach it to |
lib/internal/bootstrap/node.js
Outdated
} | ||
}); | ||
}; | ||
Object.defineProperty(global, 'queueMicrotask', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding globals is semver-major as it changes the behavior of non-strict code :/.
queueMicrotask(() => { | ||
assert.deepStrictEqual(q, ['a', 'b', 'c']); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that shows the relationship with process.nextTick
and setImmediate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setImmediate and nextTick are tested separately, and there is a PR open to change the semantics of how microtasks and timers interleave so I think it would be best to leave that alone from this side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be tested. It's fairly important that we make it explicit so we verify this all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link the PR that changes the semantics of how microtasks and timers interleave?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of this PR's intentions is interoperability... I'm not going to land it on util or process or whatever. As far as I can tell, the reason this isn't in any browsers yet is because it is fairly new. It only passed TAG review like 20 days ago. @joyeecheung posted some links above which are fairly helpful in this regard. If we choose the experimental route, I would prefer a warning. Would anyone want a flag instead? |
A warning should be the way to go. |
21ae3a5
to
d1d53d9
Compare
+1 for experimental w/warning and +1 for requiring a flag while this is still not shipped in any browsers.
Well, until this is proven out with solid implementation experience over time, I'm not sure that's the advice we should be giving generally, but I definitely get the idea. This should definitely be added to |
doc/api/globals.md
Outdated
@@ -175,6 +175,44 @@ added: v10.0.0 | |||
|
|||
The WHATWG `URLSearchParams` class. See the [`URLSearchParams`][] section. | |||
|
|||
## queueMicrotask(callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to go before ## require()
ABC-wise.
doc/api/globals.md
Outdated
|
||
* `callback` {Function} Function to be run from the [microtask queue][]. | ||
|
||
The `queueMicrotask` method queues a microtask to invoke `callback`. If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`queueMicrotask`
-> `queueMicrotask()`
here and below.
doc/api/globals.md
Outdated
* `callback` {Function} Function to be run from the [microtask queue][]. | ||
|
||
The `queueMicrotask` method queues a microtask to invoke `callback`. If | ||
`callback` throws an exception, the [`process` object][] `error` event will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`error` event
-> `'error'` event
doc/api/globals.md
Outdated
`callback` throws an exception, the [`process` object][] `error` event will be | ||
emitted. | ||
|
||
In general, `queueMicrotask` is the idiomatic choice over `process.nextTick`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`process.nextTick`
-> `process.nextTick()`
here and in mentions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to need more of an explanation as to why it is the "idiomatic choice". process.nextTick()
has been around a very very long time. Simply introducing this is not going to get users to move over to queueMicrotask()
unless the differences between the two are more spelled out. Given that users of process.nextTick()
are generally very familiar with it's timing, I'm not sure that "unexpected order" is correct.
The example is good, but including a version of the example and including a comparison of the output to help illustrate the point is better.
doc/api/globals.md
Outdated
execution order may be observed. | ||
|
||
```js | ||
// Here, `queueMicrotask` is used to ensure the load event is always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load event
-> 'load' event
doc/api/globals.md
Outdated
// jobs, resulting in inconsistent or surprising timing. | ||
|
||
DataHandler.prototype.load = async function load(key) { | ||
const d = this._cache.get(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is worth to replace d
with a more meaningful name?
lib/internal/bootstrap/node.js
Outdated
}; | ||
Object.defineProperty(global, 'queueMicrotask', { | ||
get: () => { | ||
process.emitWarning('queueMicrotask is experimental.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: queueMicrotask() is experimental
@AndreasMadsen and @trevnorris are the best to answer that question :-) |
I feel that we need to be careful about the cognitive cost that we add to our API in exchange to an utility like this. How many of our users have a clear idea about what the micro task queue is in the first place? Isn't this supposed to be an implementation detail that most users should not care about? (we are already paying the cost with the name |
Also worth reiterating: |
@joyeecheung @jasnell good points. i don't really have answers to how developer education should happen. The main point here is that when you want to do something in the next tick it should happen the same way everything else does. People who care about this timing generally use If the name itself is a problem, maybe we could talk to WHATWG about changing it? nothing else is shipping this right now so it would be fairly easy. I think the API itself stands by its own merit, regardless of the name it goes by. |
I know that this is going to sound like a strange question, but here goes: Why? At the moment, Node is steadily acquiring cruft with no discernible plan to get to a cleaner place over time. I do believe that there is a viable alternative, and I've seen it operate in practice with a popular framework and a language that has very similar characteristics to JavaScript. I sketch out the approach in The menu is omakase. I don't believe that any one change justifies the approach I've outlined. It only becomes viable if you look at the cumulative effect of both the real an cognitive costs of maintaining multiple similar APIs and/or the cost of making it more difficult or less attractive for Node to implement standards. The TL;DR version of this for the |
@rubys like i said above, part of the reason for choosing this specific api is interoperability with what the rest of the ecosystem has planned. I'm sure people will have polyfills anyway for older browsers and older node, but i think node core should ship this feature. |
This looks like it landed without a green CI. After-the-fact, but here we go anyway... Rebuild arm fanned: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3719/ Rebuild custom worker should eventually show up at: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/1096/ |
@Trott would this be a bug? neither the github ui or git-node caught these failures. |
Always best to directly check the ci page itself. Don't rely on the reporting to GitHub. It can be quite unreliable at times |
@devsnek
|
Arm rebuild is green. Custom/worker rebuild is red but I'm not sure why, at least not after a quick glance. Anyone know? https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/1096/ |
|
So that failure in workers is related to this change. This needs to be reverted or fixed. |
It's because that commit adds a new module loaded at node.js start up, which can vary from platform to platform and on how node.js is started. It's not a great test and is very brittle |
Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)
Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)
queueMicrotask
is a global from browsers which enables the user code to insert a callback into the microtask queue. We currently do not expose this functionality.Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-queuemicrotask
/cc @nodejs/timers @nodejs/open-standards @domenic
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes