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 napi_make_callback documentation #35321

Closed
wants to merge 1 commit into from
Closed

doc: update napi_make_callback documentation #35321

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Sep 23, 2020

Calling napi_make_callback() with no async_context is not resulting in using the current async context instead an empty context (id 0) is used.

Using NULL is like using node::Makecallback without async_context which is deprecated since Node.js 10 (DEP0099).

fixes: #35188

@Flarna
doc: update napi_make_callback documentation
b971b46 
Calling napi_make_callback() with no async_context is not resulting in
using the current async context instead an empty context (id 0) is
used.

Using NULL is like using node::Makecallback without async_context which
is deprecated since Node.js 10 (DEP0099).
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 23, 2020 •
edited by mhdawson
Loading

Review requested:

  • @nodejs/n-api

@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 Sep 23, 2020
@legendecas
Copy link
Member

@Flarna you are suggesting that this should be a doc issue rather than actual bug that should be fixed, am I got it right? As per #35188 (comment) IIUC this can be considered a bug that needs to be fixed in the code (and backported).

@Flarna
Copy link
Member Author

Flarna commented Sep 24, 2020

napi_make_callback() is a wrapper around node::Makecallback and it behaves like node::Makecallback. I think it would be quite confusing to have a these two functions behave different.

I know there is a subtle difference: node::Makecallback used without async_context results in a compile time deprecation warning whereas napi doesn't. But I think this can't be synced as it is not possible in napi to deprecate something (or not within a patch/minor version). Don't know how deprecation in napi works as the target is a stable API across several major versions.

I'm not even sure if this can be fixed in code. It would require to "detect" the current context and use it without calling any async hook callback. I see the risk that such a "fix" changes the behavior of a lot addons in the wild.

legendecas
legendecas approved these changes Sep 25, 2020
View reviewed changes
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
mhdawson approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Flarna Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 25, 2020
@mhdawson
Copy link
Member

Doc only so linters already run are good enough, landing

mhdawson pushed a commit that referenced this pull request Sep 25, 2020
@Flarna @mhdawson
doc: update napi_make_callback documentation
03c4ee9 
Calling napi_make_callback() with no async_context is not resulting in
using the current async context instead an empty context (id 0) is
used.

Using NULL is like using node::Makecallback without async_context which
is deprecated since Node.js 10 (DEP0099).

PR-URL: #35321
Fixes: #35188
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed in 03c4ee9

@mhdawson mhdawson closed this Sep 25, 2020
@Flarna Flarna deleted the doc_napi_make_callback branch September 25, 2020 21:52
MylesBorins pushed a commit that referenced this pull request Sep 29, 2020
@Flarna @MylesBorins
doc: update napi_make_callback documentation
0a847ca 
Calling napi_make_callback() with no async_context is not resulting in
using the current async context instead an empty context (id 0) is
used.

Using NULL is like using node::Makecallback without async_context which
is deprecated since Node.js 10 (DEP0099).

PR-URL: #35321
Fixes: #35188
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 29, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@Flarna @joesepi
doc: update napi_make_callback documentation
b4a0b07 
Calling napi_make_callback() with no async_context is not resulting in
using the current async context instead an empty context (id 0) is
used.

Using NULL is like using node::Makecallback without async_context which
is deprecated since Node.js 10 (DEP0099).

PR-URL: nodejs#35321
Fixes: nodejs#35188
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@addaleax addaleax addaleax approved these changes

@NickNaso NickNaso NickNaso approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

napi_make_callback() with null async_context doesn't reuse current context
7 participants
@Flarna @nodejs-github-bot @legendecas @mhdawson @NickNaso @addaleax @gabrielschulhof