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: update async_hooks providers list #13561

Closed
wants to merge 1 commit into from
Closed

doc: update async_hooks providers list #13561

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 8, 2017

Ref: #13452

Checklist
Affected core subsystem(s)

doc/async_hooks

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. labels Jun 8, 2017
refack
refack approved these changes Jun 8, 2017
View reviewed changes
doc/api/async_hooks.md Outdated

There is also the `PROMISE` resource type, which is used to track `Promise`
instances and asynchronous work scheduled by them.
`init` to call. Generally it will correspond the name of the resource's
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it was there before but probably should to call -> to be called
Less sure but maybe correspond -> correlate
Correspond: have a close similarity; match or agree almost exactly.
Correlate: have a mutual relationship or connection, in which one thing affects or depends on another.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it was there before but probably should to call -> to be called

Ah right, done.

Correspond: have a close similarity; match or agree almost exactly.

Well, that’s basically exactly what it’s supposed to say, so it seems like choosing correspond is perfect. :)

@trevnorris
Copy link
Contributor

Don't agree with this changed for same reasons stated in #13452 (comment). Users should know what they can filter for from node core, and should also be able to find the same in documentation from module authors that create their own types.

@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2017

@nodejs/async_hooks … I don’t have terribly strong feelings either way.

@Fishrock123
Copy link
Contributor

Why not update this rather than remove it? 😕

@refack
Copy link
Contributor

refack commented Jun 8, 2017

Should we just expose process.binding('async_wrap').Providers?

@addaleax addaleax force-pushed the async-hooks-doc-remove-type-list branch from e9178fe to a9eb903 Compare June 8, 2017 22:40
@addaleax addaleax changed the title doc: remove async_hooks providers list doc: update async_hooks providers list Jun 8, 2017
@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2017

@trevnorris @Fishrock123 Seems like I misread the discussion in #13452 a bit. I’ve updated this to add Timeout and Immediate instead.

mscdex
mscdex reviewed Jun 8, 2017
View reviewed changes
doc/api/async_hooks.md Outdated

```
FSEVENTWRAP, FSREQWRAP, GETADDRINFOREQWRAP, GETNAMEINFOREQWRAP, HTTPPARSER,
JSSTREAM, PIPECONNECTWRAP, PIPEWRAP, PROCESSWRAP, QUERYWRAP, SHUTDOWNWRAP,
SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPWRAP, TIMERWRAP, TTYWRAP,
UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST,
RANDOMBYTESREQUEST, TLSWRAP
RANDOMBYTESREQUEST, TLSWRAP, Timeout, Immediate
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't Timeout and Immediate be uppercase or no?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, these values are emitted from JS and are written this way in the source code.

Copy link
Member

Choose a reason for hiding this comment

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

It may be worth adding a note to point that out ... specifically, the all-caps ones come from native code while the mixed case come from JavaScript

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget TickObject! 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

@trevnorris Added. :)

@addaleax addaleax force-pushed the async-hooks-doc-remove-type-list branch from a9eb903 to 4bbad95 Compare June 10, 2017 10:11
@trevnorris
Copy link
Contributor

@refack

Should we just expose process.binding('async_wrap').Providers?

I'm down for that, but please expose it as types (or Types) instead. "Providers" is for legacy reasons and I never liked that name.

@refack refack mentioned this pull request Jun 11, 2017
Closed
4 tasks
@refack
Copy link
Contributor

refack commented Jun 11, 2017

I'm down for that, but please expose it as types (or Types) instead. "Providers" is for legacy reasons and I never liked that name.

Ref: #13610

jasnell pushed a commit that referenced this pull request Jun 13, 2017
@addaleax @jasnell
doc: update async_hooks providers list
0ab4614 
PR-URL: #13561
Ref: #13452
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 13, 2017

Landed in 0ab4614

@jasnell jasnell closed this Jun 13, 2017
addaleax added a commit that referenced this pull request Jun 17, 2017
@addaleax
doc: update async_hooks providers list
4298f9e 
PR-URL: #13561
Ref: #13452
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 17, 2017
Merged
addaleax added a commit that referenced this pull request Jun 21, 2017
@addaleax
doc: update async_hooks providers list
9b74dde 
PR-URL: #13561
Ref: #13452
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell approved these changes

@trevnorris trevnorris trevnorris approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @trevnorris @Fishrock123 @refack @jasnell @mscdex @MylesBorins @nodejs-github-bot