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

node-api: define version 10 #55676

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Nov 1, 2024

Notable runtime changes to existing APIs:

  • returning node_api_cannot_run_js instead of napi_pending_exception.
  • allow creating references to objects, functions, and symbols.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 1, 2024
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Nov 1, 2024
@gabrielschulhof
Copy link
Contributor Author

To discuss: --force-node-api-uncaught-exceptions-policy

@gabrielschulhof gabrielschulhof marked this pull request as ready for review November 15, 2024 03:44
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (49acdc8) to head (e9de692).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/node_api.cc 0.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55676   +/-   ##
=======================================
  Coverage   88.54%   88.55%           
=======================================
  Files         657      657           
  Lines      190655   190657    +2     
  Branches    36582    36585    +3     
=======================================
+ Hits       168824   168829    +5     
+ Misses      15002    15001    -1     
+ Partials     6829     6827    -2     
Files with missing lines Coverage Δ
src/js_native_api_v8.cc 76.51% <100.00%> (ø)
src/js_native_api_v8.h 90.90% <ø> (ø)
src/node_api.cc 76.13% <0.00%> (-0.19%) ⬇️

... and 22 files with indirect coverage changes

@gabrielschulhof gabrielschulhof force-pushed the node-api-10-proposal branch 3 times, most recently from c8e1c65 to 66d9b4e Compare November 15, 2024 15:48
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.

Should

if (env->module_api_version < NAPI_VERSION_EXPERIMENTAL &&
(https://github.com/nodejs/node/blob/main/doc/api/n-api.md#napi_create_threadsafe_function uncaught exception behavior) be included in version 10 as well?

@legendecas legendecas self-requested a review November 28, 2024 15:46
Notable runtime changes to existing APIs:
- returning `node_api_cannot_run_js` instead of
  `napi_pending_exception`.
- allow creating references to objects, functions, and symbols.
@gabrielschulhof
Copy link
Contributor Author

@legendecas updated the PR. Please take another look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants