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.

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

feat: expose async wrap providers under async_hooks module #40760

Merged

Conversation

@RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Nov 8, 2021

Address: #40730.

doc/api/async_hooks.md Outdated Show resolved Hide resolved
Loading
lib/internal/async_hooks.js Show resolved Hide resolved
Loading
doc/api/async_hooks.md Outdated Show resolved Hide resolved
Loading
@mcollina
Copy link
Member

@mcollina mcollina commented Nov 8, 2021

cc @jasnell @addaleax

we need this to support Bubbleprof without the deprecation notice.

Loading

doc/api/async_hooks.md Outdated Show resolved Hide resolved
Loading
added: REPLACEME
-->

* Returns: A list of providers supported by async wrap.
Copy link
Member

@targos targos Nov 9, 2021

Choose a reason for hiding this comment

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

Do we define anywhere what a "provider" is?
Also, to me "A list" means an array, but an object is returned.

Loading

Copy link
Member Author

@RafaelGSS RafaelGSS Nov 9, 2021

Choose a reason for hiding this comment

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

I think that it wasn't defined, at least publically. I can reword that for sure, but, to be honest, I don't know how to define the providers description.

Loading

Copy link
Member

@Qard Qard Nov 10, 2021

Choose a reason for hiding this comment

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

Suggested change
* Returns: A list of providers supported by async wrap.
* Returns: A map of provider types supported by async wrap.

I would also add something to the description linking it to type parameter of the init(...) hook.

Loading

Copy link
Member Author

@RafaelGSS RafaelGSS Nov 10, 2021

Choose a reason for hiding this comment

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

Just did, thanks for point that out!

Loading

@RafaelGSS RafaelGSS requested review from Qard, targos and mcollina Nov 11, 2021
Copy link
Member

@Qard Qard left a comment

Just a few nits

Loading

doc/api/async_hooks.md Outdated Show resolved Hide resolved
Loading
lib/async_hooks.js Show resolved Hide resolved
Loading
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 11, 2021

Loading

Copy link
Member

@mcollina mcollina left a comment

lgtm

Loading

doc/api/async_hooks.md Outdated Show resolved Hide resolved
Loading
lib/async_hooks.js Outdated Show resolved Hide resolved
Loading
@nodejs-github-bot

This comment has been hidden.

@nodejs-github-bot

This comment has been hidden.

Flarna
Flarna approved these changes Nov 11, 2021
* Returns: A map of provider types to the corresponding numeric id.
This map contains all the event types that might be emitted by the `async_hooks.init()` event.

This feature suppresses the deprecated usage of `process.binding('async_wrap').Providers`.
Copy link
Member

@Flarna Flarna Nov 11, 2021

Choose a reason for hiding this comment

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

Maybe add a link to DEP0111

Loading

@Flarna
Copy link
Member

@Flarna Flarna commented Nov 11, 2021

Nit: My understaning of the commit message guideline tells that the commit message should start with async_hooks.

Loading

@Qard
Copy link
Member

@Qard Qard commented Nov 11, 2021

I would just squash it all before merging and give it the async_hooks subsystem prefix then.

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 11, 2021

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 12, 2021

Loading

@RafaelGSS RafaelGSS force-pushed the feat/expose-async-wrap-providers branch from f95a743 to 29ad912 Nov 12, 2021
docs: add asyncWrapProviders api doc

tests(async_hooks): use internalBinding for comparisson

fix(test-async-wrap): lint error

docs: use REPLACEME for asyncWrapProviders

update: use freeze and copy for asyncWrapProviders

update(async_hooks): use primordials on asyncWrapProviders

fix: use common to expect error

docs(asyncWrapProviders): rephrase return type

fix: lint md

fix: lint md

docs(async_hooks): typo

Co-authored-by: Stephen Belanger <admin@stephenbelanger.com>

update(asyncWrapProviders): add __proto__ as null

Co-authored-by: Simone Busoli <simone.busoli@gmail.com>
Co-authored-by: Michaël Zasso <targos@protonmail.com>

test: adjust __proto__ assertion

docs: add DEP0111 link
@RafaelGSS RafaelGSS force-pushed the feat/expose-async-wrap-providers branch from 29ad912 to c19f6e2 Nov 12, 2021
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 12, 2021

Loading

@RafaelGSS RafaelGSS requested a review from Qard Nov 12, 2021
Qard
Qard approved these changes Nov 12, 2021
@RafaelGSS RafaelGSS requested a review from targos Nov 12, 2021
@nodejs-github-bot nodejs-github-bot merged commit d10085b into nodejs:master Nov 12, 2021
51 checks passed
Loading
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 12, 2021

Landed in d10085b

Loading

targos added a commit that referenced this issue Nov 21, 2021
docs: add asyncWrapProviders api doc

tests(async_hooks): use internalBinding for comparisson

fix(test-async-wrap): lint error

docs: use REPLACEME for asyncWrapProviders

update: use freeze and copy for asyncWrapProviders

update(async_hooks): use primordials on asyncWrapProviders

fix: use common to expect error

docs(asyncWrapProviders): rephrase return type

fix: lint md

fix: lint md

docs(async_hooks): typo

Co-authored-by: Stephen Belanger <admin@stephenbelanger.com>

update(asyncWrapProviders): add __proto__ as null

Co-authored-by: Simone Busoli <simone.busoli@gmail.com>
Co-authored-by: Michaël Zasso <targos@protonmail.com>

test: adjust __proto__ assertion

docs: add DEP0111 link

PR-URL: #40760
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
targos added a commit that referenced this issue Nov 26, 2021
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) #40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858

PR-URL: #40983
targos added a commit that referenced this issue Nov 29, 2021
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) #40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858

PR-URL: #40983
targos added a commit that referenced this issue Nov 30, 2021
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) #40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858

PR-URL: #40983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants