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

doc: update napi_async_init documentation #33181

Closed
wants to merge 4 commits into from

Conversation

mhdawson
Copy link
Member

Fixes: #33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Fixes: nodejs#33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <[email protected]>
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Apr 30, 2020
@mhdawson mhdawson marked this pull request as draft April 30, 2020 23:00
doc/api/n-api.md Outdated Show resolved Hide resolved
Co-authored-by: legendecas <[email protected]>
doc/api/n-api.md Outdated Show resolved Hide resolved
@mhdawson mhdawson marked this pull request as ready for review May 4, 2020 15:51
@mhdawson
Copy link
Member Author

mhdawson commented May 4, 2020

@legendecas addressed your comment.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member Author

mhdawson commented May 6, 2020

Doc only and liters were ok so landing.

@mhdawson
Copy link
Member Author

mhdawson commented May 6, 2020

Landed in 3662b0c

@mhdawson mhdawson closed this May 6, 2020
mhdawson added a commit that referenced this pull request May 6, 2020
Fixes: #33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #33181
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
codebytere pushed a commit that referenced this pull request May 7, 2020
Fixes: #33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #33181
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@codebytere codebytere mentioned this pull request May 18, 2020
codebytere pushed a commit that referenced this pull request Jun 7, 2020
Fixes: #33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #33181
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@codebytere codebytere mentioned this pull request Jun 9, 2020
@mhdawson mhdawson deleted the napi-init branch September 14, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource is optional in napi_async_init
4 participants