-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: clarify that N-API addons are context-aware #36640
Conversation
The docs on N-API say that NAPI_MODULE_INIT must be used for the addon to be context-aware. That seems to be wrong, i.e. all N-API addons are context-aware(?)
@nodejs/n-api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm blocking solely because many have already approved, and the change really should say NAPI_MODULE
, not NODE_MODULE
.
If the module will be loaded multiple times during the lifetime of the Node.js | ||
process, use the `NAPI_MODULE_INIT` macro to initialize the module: | ||
You can also use the `NAPI_MODULE_INIT` macro, which acts as a shorthand | ||
for `NODE_MODULE` and defining an `Init` function: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for `NODE_MODULE` and defining an `Init` function: | |
for `NAPI_MODULE` and defining an `Init` function: |
Otherwise, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @gabrielschulhof comment is addresssed.
I think we can fix this as we land it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Landing with the fix...
The docs on N-API say that NAPI_MODULE_INIT must be used for the addon to be context-aware. That seems to be wrong, i.e. all N-API addons are context-aware(?) PR-URL: #36640 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Landed in a3fcf24. |
The docs on N-API say that NAPI_MODULE_INIT must be used for the addon to be context-aware. That seems to be wrong, i.e. all N-API addons are context-aware(?) PR-URL: #36640 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
The docs on N-API say that NAPI_MODULE_INIT must be used for the addon to be context-aware. That seems to be wrong, i.e. all N-API addons are context-aware(?) PR-URL: #36640 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
The docs on N-API say that NAPI_MODULE_INIT must be used for the addon to be context-aware. That seems to be wrong, i.e. all N-API addons are context-aware(?)