async_hooks: docs and api ergonomics tweaks by jasnell · Pull Request #15103 · nodejs/node

jasnell · 2017-08-30T22:17:14Z

Some doc improvements and a AsyncResource constructor ergonomics tweak...

Previously, if type was passed to the AsyncResource class as null or undefined,
an error would be thrown. With this, type defaults to new.target.name.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

refack

Code LGTM.
Semi-Rubberstamp the docs.

refack · 2017-08-30T22:27:43Z

/cc @nodejs/async_hooks

maclover7

Left a few possible changes about the docs

maclover7 · 2017-08-30T22:41:43Z

doc/api/async_hooks.md

that isn't needed anymore

maclover7 · 2017-08-30T22:42:29Z

doc/api/async_hooks.md

maybe swap out it for resource? was slightly unclear what it was referring to

maclover7 · 2017-08-30T22:44:43Z

doc/api/async_hooks.md

I think a comma is needed after not depend on garbage collection

maclover7 · 2017-08-30T22:45:35Z

doc/api/async_hooks.md

ascyc --> async

maclover7 · 2017-08-30T22:48:17Z

doc/api/async_hooks.md

Not completely sure about this one, but should this be The following? 😬

AndreasMadsen

I'm not so sure about the type default.

AndreasMadsen · 2017-08-31T06:31:46Z

lib/async_hooks.js

This won't work with async_hooks: expose list of types #13610. /cc @refack

If the user types new AsyncResource() the name will conflict with another module.

You can use type = new.target.name in the constructor.

If we don't default then we should TypeError if the type isn't provided rather than the more obscure error that throws now.

throw `TypeError` if `type` is not provided or is not a string

jasnell · 2017-09-01T18:19:54Z

@AndreasMadsen ... ok, I've reworked this to take the approach of throwing instead of defaulting. PTAL.

@refack ... please take another look also.

AndreasMadsen

LGTM

jasnell · 2017-09-05T17:41:40Z

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

refack · 2017-09-05T18:26:47Z

This won't work with async_hooks: expose list of types #13610.

If the user types new AsyncResource() the name will conflict with another module.

You can use type = new.target.name in the constructor.

We could do some behind the scenes juggling like if (new.target.name === 'AsyncResource') throw "AsyncResource is an abstract class";
And lazy register the name from the constructor in a #13610 like mechanism.

But that could wait for a future PR. Explicit type is a decent solution, considering it's the only discriminating property of the hooked events.

BridgeAR · 2017-09-11T04:15:23Z

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

jasnell · 2017-09-13T16:20:26Z

With the change to the error handling, this actually becomes a semver-major.

@addaleax and @trevnorris ... I'd appreciate a review on this.

BridgeAR · 2017-09-13T16:41:59Z

@jasnell async_hooks is still experimental. Therefore this should not be semver-major.

jasnell · 2017-09-13T16:43:27Z

Yes, technically that's correct, but given that they've been around for a while now, I think it's prudent to be careful

trevnorris

have a couple questions, but nothing that needs to be done.

trevnorris · 2017-09-14T19:23:48Z

doc/api/async_hooks.md

 it is thus not safe to use it as a key in a `WeakMap` or add properties to it.

-###### asynchronous context example
+###### Asynchronous context example


not sure of any current style, but should headers capitalize all words? doesn't matter to me either way.

heck if I know. I don't think we've had much of a consistent style here.

trevnorris · 2017-09-14T19:24:39Z

doc/api/async_hooks.md

 `console.log()` being called. This is because binding to a port without a
-hostname is actually synchronous, but to maintain a completely asynchronous API
-the user's callback is placed in a `process.nextTick()`.
+hostname is a *synchronous* operation within Node.js, but to maintain a


is "within Node.js" necessary? possibly we're referring to something else, but i'm not sure what.

Likely not necessary.

trevnorris · 2017-09-14T19:30:04Z

doc/api/async_hooks.md

-appropriate callbacks are called. To accommodate this a JavaScript API is
-provided.
+Library developers that handle their own asychronous resources performing tasks
+like I/O, connection pooling, or managing callback queues may use the AsyncWrap


should AsyncWrap be placed in backticks?

jasnell · 2017-09-14T23:27:07Z

@nodejs/tsc ... this needs another tsc member to review (unless folks are ok with it not being semver-major)

mcollina

LGTM as semver-minor.
I prefer this to land in 8 than not have it in there.

What's the plan for moving async-hooks out of experimental?

AndreasMadsen · 2017-09-15T11:29:14Z

What's the plan for moving async-hooks out of experimental?

@mcollina Let's have the discussion in #14717. I've made a comment on what I think is required #14717 (comment).

Update docs and type checking for AsyncResource type PR-URL: #15103 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-09-15T16:23:18Z

Landed in d8a0364

Update docs and type checking for AsyncResource type PR-URL: nodejs/node#15103 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Update docs and type checking for AsyncResource type PR-URL: #15103 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Update docs and type checking for AsyncResource type PR-URL: nodejs/node#15103 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Aug 30, 2017

jasnell requested review from addaleax and trevnorris August 30, 2017 22:17

jasnell changed the title ~~Async hooks tweaks~~ async_hooks: docs and api ergonomics tweaks Aug 30, 2017

refack approved these changes Aug 30, 2017

View reviewed changes

maclover7 reviewed Aug 30, 2017

View reviewed changes

AndreasMadsen suggested changes Aug 31, 2017

View reviewed changes

AndreasMadsen added dont-land-on-v4.x labels Aug 31, 2017

jasnell added 2 commits September 1, 2017 11:19

doc: some async_hooks docs improvements

73b8272

async_hooks: add type checking for AsyncResource type

def3c83

throw `TypeError` if `type` is not provided or is not a string

jasnell force-pushed the async_hooks_tweaks branch 2 times, most recently from 9c2f1d5 to def3c83 Compare September 1, 2017 18:19

[Squash] Doc nits

f1cdae3

AndreasMadsen approved these changes Sep 2, 2017

View reviewed changes

refack approved these changes Sep 5, 2017

View reviewed changes

[Squash] Update tests

b4a1075

BridgeAR approved these changes Sep 8, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 13, 2017

trevnorris approved these changes Sep 14, 2017

View reviewed changes

[Squash] Nits

8f032cf

mcollina approved these changes Sep 15, 2017

View reviewed changes

AndreasMadsen mentioned this pull request Sep 15, 2017

async_hooks experimental status #14717

Closed

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 15, 2017

jasnell closed this Sep 15, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

Uh oh!

Conversation

jasnell commented Aug 30, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 30, 2017

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 1, 2017

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 5, 2017

Uh oh!

refack commented Sep 5, 2017

Uh oh!

BridgeAR commented Sep 11, 2017

Uh oh!

jasnell commented Sep 13, 2017

Uh oh!

BridgeAR commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 13, 2017

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Sep 15, 2017

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

BridgeAR commented Sep 13, 2017 •

edited

Loading

jasnell commented Sep 14, 2017 •

edited

Loading