async_hooks: rename PromiseWrap.parentId #18633

ofrobots · 2018-02-08T01:49:44Z

Fixes: #18470

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc: async_hooks

/cc @nodejs/async_hooks

Trott · 2018-02-08T04:30:30Z

doc/api/async_hooks.md

@@ -304,7 +304,9 @@ In the case of Promises, the `resource` object will have `promise` property
 that refers to the Promise that is being initialized, and a `parentId` property
 set to the `asyncId` of a parent Promise, if there is one, and `undefined`
 otherwise. For example, in the case of `b = a.then(handler)`, `a` is considered
-a parent Promise of `b`.
+a parent Promise of `b`. Note that, in the cases where `parentId` is defined, it


Nit: Get rid of Note that, in the cases where and replace it with If or When so the sentence might start:

If `parentId` is defined, it...

AndreasMadsen · 2018-02-08T07:58:32Z

Given that there is no one using this API at the moment, I would actually prefer a breaking change.

JiaLiPassion · 2018-02-08T08:05:53Z

I agree with @AndreasMadsen , a breaking change is more make sense.

BridgeAR · 2018-02-08T11:48:33Z

@ofrobots please always trigger a CI (mini in this case) after creating a PR :-)

Mini-CI https://ci.nodejs.org/job/node-test-commit-light/234/

That aside: I also think it would be better to have a breaking change in this case.

ofrobots · 2018-02-08T15:24:44Z

@AndreasMadsen

Given that there is no one using this API at the moment

Do we know this? I'm on board if that's the case.

ofrobots · 2018-02-08T18:00:19Z

I went looking for known users of async_hooks. These modules do not use parentId AFAICT.

The following did come up with a use; but I don't think it is a real use.

cls-hooked seems to use parentId but only in a debug log statement. /cc @Jeff-Lewis.

Based on this, I think it is reasonable to go ahead and make a breaking change.

ofrobots · 2018-02-09T21:25:38Z

Changed to a breaking change as recommended. Since async_hooks is experimental, this can be considered a semver-minor.

This will require a re-review. Thanks in advance!

@AndreasMadsen @nodejs/async_hooks

significant changes made

AndreasMadsen

LGTM

BridgeAR · 2018-02-10T18:58:46Z

CI https://ci.nodejs.org/job/node-test-pull-request/13088/

Rename the `parentId` property on the PromiseWrap object to a `isChainedPromise` property. The former wasn't quite useful as it was always defined to be the same value as the trigger id available in the init hook. Instead rename the property to be closer to the information it communicates: whether the promise is a chained promise or not. PR-URL: nodejs#18633 Fixes: nodejs#18470 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

ofrobots · 2018-02-12T12:28:54Z

Thanks! Landed as 9f6a565.

Rename the `parentId` property on the PromiseWrap object to a `isChainedPromise` property. The former wasn't quite useful as it was always defined to be the same value as the trigger id available in the init hook. Instead rename the property to be closer to the information it communicates: whether the promise is a chained promise or not. PR-URL: #18633 Fixes: #18470 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

Rename the `parentId` property on the PromiseWrap object to a `isChainedPromise` property. The former wasn't quite useful as it was always defined to be the same value as the trigger id available in the init hook. Instead rename the property to be closer to the information it communicates: whether the promise is a chained promise or not. PR-URL: nodejs#18633 Fixes: nodejs#18470 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

MylesBorins · 2018-08-07T18:57:45Z

@ofrobots I've backported this to v8.x, please lmk if it should be backed out

Rename the `parentId` property on the PromiseWrap object to a `isChainedPromise` property. The former wasn't quite useful as it was always defined to be the same value as the trigger id available in the init hook. Instead rename the property to be closer to the information it communicates: whether the promise is a chained promise or not. PR-URL: #18633 Fixes: #18470 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added async_hooks doc labels Feb 8, 2018

ofrobots mentioned this pull request Feb 8, 2018

async_hooks: for promises is PromiseWrap::parentId actually useful? #18470

Closed

Trott reviewed Feb 8, 2018

View changes

Trott previously approved these changes Feb 8, 2018

View changes

cjihrig previously approved these changes Feb 8, 2018

View changes

ofrobots force-pushed the doc-async-hooks-promise-id branch 2 times, most recently from e75c848 to 1231f99 Compare Feb 9, 2018

ofrobots added the semver-minor label Feb 9, 2018

ofrobots changed the title ~~doc: async_hooks: explain parentId more~~ async_hooks: rename PromiseWrap.parentId Feb 9, 2018

AndreasMadsen approved these changes Feb 9, 2018

View changes

jasnell approved these changes Feb 9, 2018

View changes

BridgeAR added the author ready label Feb 10, 2018

TimothyGu approved these changes Feb 10, 2018

View changes

ofrobots force-pushed the doc-async-hooks-promise-id branch from 1231f99 to 9f6a565 Compare Feb 12, 2018

ofrobots merged commit 9f6a565 into nodejs:master Feb 12, 2018

ofrobots deleted the doc-async-hooks-promise-id branch Feb 12, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

joshdover mentioned this pull request Oct 10, 2018

Bump Node to 8.12.0 elastic/kibana#23559

Closed

nodejs / node Public

async_hooks: rename PromiseWrap.parentId #18633

async_hooks: rename PromiseWrap.parentId #18633

ofrobots commented Feb 8, 2018

Trott Feb 8, 2018 •

edited

ofrobots Feb 8, 2018

AndreasMadsen commented Feb 8, 2018

JiaLiPassion commented Feb 8, 2018

BridgeAR commented Feb 8, 2018

ofrobots commented Feb 8, 2018

ofrobots commented Feb 8, 2018 •

edited

ofrobots commented Feb 9, 2018 •

edited

AndreasMadsen left a comment

BridgeAR commented Feb 10, 2018

ofrobots commented Feb 12, 2018

MylesBorins commented Aug 7, 2018

nodejs / node Public

async_hooks: rename PromiseWrap.parentId #18633

async_hooks: rename PromiseWrap.parentId #18633

Conversation

ofrobots commented Feb 8, 2018

Checklist

Affected core subsystem(s)

Trott Feb 8, 2018 • edited

Choose a reason for hiding this comment

ofrobots Feb 8, 2018

Choose a reason for hiding this comment

AndreasMadsen commented Feb 8, 2018

JiaLiPassion commented Feb 8, 2018

BridgeAR commented Feb 8, 2018

ofrobots commented Feb 8, 2018

ofrobots commented Feb 8, 2018 • edited

ofrobots commented Feb 9, 2018 • edited

AndreasMadsen left a comment

BridgeAR commented Feb 10, 2018

ofrobots commented Feb 12, 2018

MylesBorins commented Aug 7, 2018

Trott Feb 8, 2018 •

edited

ofrobots commented Feb 8, 2018 •

edited

ofrobots commented Feb 9, 2018 •

edited