Skip to content

doc: remove "idiomatic choice" from queueMicrotask#23885

Closed
rvagg wants to merge 2 commits intonodejs:masterfrom
rvagg:rvagg/queue-microtask-not-idiomatic
Closed

doc: remove "idiomatic choice" from queueMicrotask#23885
rvagg wants to merge 2 commits intonodejs:masterfrom
rvagg:rvagg/queue-microtask-not-idiomatic

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

nodejs-github-bot commented Oct 25, 2018

@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
Copy link
Contributor

@apapirovski apapirovski Oct 26, 2018

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

@vsemozhetbyt vsemozhetbyt Oct 27, 2018

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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@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

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@trivikr trivikr trivikr approved these changes

+5 more reviewers

@apapirovski apapirovski apapirovski left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

@hiroppy hiroppy hiroppy approved these changes

@mmarchini mmarchini mmarchini approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

Reviewers whose approvals may not affect merge requirements

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.