Skip to content
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.

Sign up for GitHub

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

Jump to bottom

doc: remove "idiomatic choice" from queueMicrotask #23885

Closed
wants to merge 2 commits into from
Closed

doc: remove "idiomatic choice" from queueMicrotask #23885

wants to merge 2 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 25, 2018

It can't be idiomatic if it's not in general use and therefore hasn't
been picked up by users. It's not even in browsers yet.

"Idiomatic" use is an emergent property that comes from observed use
and this feature is so new (to browsers and Node) that it can't
possibly be. In general I don't think it's the place of the Node API
docs to observe what emerges as idiomatic Node.js.

It also can't be a recommended feature (if that was the intent of the
language) because it's marked experimental. For now, it's just a
feature, nothing more. Recommendations and/or observations about it
being 'idiomatic' can come later.

@nodejs-github-bot
Copy link
Collaborator

@rvagg build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1374/pipeline

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 25, 2018
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 26, 2018
apapirovski
apapirovski reviewed Oct 26, 2018
View reviewed changes
doc/api/globals.md Outdated
@@ -122,7 +122,6 @@ The `queueMicrotask()` method queues a microtask to invoke `callback`. If
`callback` throws an exception, the [`process` object][] `'uncaughtException'`
event will be emitted.

In general, `queueMicrotask` is the idiomatic choice over `process.nextTick()`.
`process.nextTick()` will always run before the microtask queue, and so
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this whole paragraph is kinda sketchy. There is nothing unexpected about the nextTick execution order. As for Node 10 it is entirely predictable and defined, with no weird edge cases. It might not be "desired" but that's up to the individual user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong, but the transition from the previous paragraph to the process.nextTick() mention seems a bit abrupt and unexpected now. Maybe we can add some smooth connection? Like On the other hand..., Similar `process.nextTick()`... etc?

@rvagg
doc: remove "idiomatic choice" from queueMicrotask
385510b 
It can't be idiomatic if it's not in general use and therefore hasn't
been picked up by users. It's not even in browsers yet.

"Idiomatic" use is an emergent property that comes from observed use
and this feature is so new (to browsers and Node) that it can't
possibly be. In general I don't think it's the place of the Node API
docs to observe what emerges as idiomatic Node.js.

It also can't be a recommended feature (if that was the intent of the
language) because it's marked experimental. For now, it's just a
feature, nothing more. Recommendations and/or observations about it
being 'idiomatic' can come later.
@rvagg rvagg force-pushed the rvagg/queue-microtask-not-idiomatic branch from d1fd048 to 999ddf4 Compare October 29, 2018 09:17
@rvagg rvagg force-pushed the rvagg/queue-microtask-not-idiomatic branch from 999ddf4 to a6da5fd Compare October 29, 2018 09:21
@rvagg
Copy link
Member Author

rvagg commented Oct 29, 2018

@apapirovski & @vsemozhetbyt good point on the flow into that second paragraph. I've expanded it to be a bit more helpful and the latest commit reads:

The queueMicrotask() method queues a microtask to invoke callback. If
callback throws an exception, the [process object][] 'uncaughtException'
event will be emitted.

The microtask queue is managed by V8 and may be used in a similar manner to
the process.nextTick() queue, which is managed by Node.js. The
process.nextTick() queue is always processed before the microtask queue
within each turn of the Node.js event loop.

Thoughts?

@rvagg rvagg closed this Oct 30, 2018
@rvagg rvagg deleted the rvagg/queue-microtask-not-idiomatic branch October 30, 2018 20:41
@rvagg
Copy link
Member Author

rvagg commented Oct 30, 2018

ad670fa

rvagg added a commit that referenced this pull request Oct 30, 2018
@rvagg
doc: remove "idiomatic choice" from queueMicrotask
ad670fa 
It can't be idiomatic if it's not in general use and therefore hasn't
been picked up by users. It's not even in browsers yet.

"Idiomatic" use is an emergent property that comes from observed use
and this feature is so new (to browsers and Node) that it can't
possibly be. In general I don't think it's the place of the Node API
docs to observe what emerges as idiomatic Node.js.

It also can't be a recommended feature (if that was the intent of the
language) because it's marked experimental. For now, it's just a
feature, nothing more. Recommendations and/or observations about it
being 'idiomatic' can come later.

PR-URL: #23885
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Nov 1, 2018
@rvagg @targos
doc: remove "idiomatic choice" from queueMicrotask
ee299c7 
It can't be idiomatic if it's not in general use and therefore hasn't
been picked up by users. It's not even in browsers yet.

"Idiomatic" use is an emergent property that comes from observed use
and this feature is so new (to browsers and Node) that it can't
possibly be. In general I don't think it's the place of the Node API
docs to observe what emerges as idiomatic Node.js.

It also can't be a recommended feature (if that was the intent of the
language) because it's marked experimental. For now, it's just a
feature, nothing more. Recommendations and/or observations about it
being 'idiomatic' can come later.

PR-URL: #23885
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@targos targos mentioned this pull request Nov 1, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski left review comments

@trivikr trivikr trivikr approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@mmarchini mmarchini mmarchini approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.