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

Merged
merged 2 commits into from
Dec 31, 2024

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 28 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!

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2024
@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof gabrielschulhof added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 30, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 30, 2024
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55676
✔  Done loading data for nodejs/node/pull/55676
----------------------------------- PR info ------------------------------------
Title      node-api: define version 10 (#55676)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     gabrielschulhof:node-api-10-proposal -> nodejs:main
Labels     c++, lib / src, node-api, needs-ci
Commits    2
 - node-api: define version 10
 - fixup! TSFN fatal uncaught exception handling
Committers 1
 - Gabriel Schulhof <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55676
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55676
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 01 Nov 2024 14:47:59 GMT
   ✔  Approvals: 2
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/55676#pullrequestreview-2522671800
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55676#pullrequestreview-2524627055
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-12-25T23:40:30Z: https://ci.nodejs.org/job/node-test-pull-request/64211/
- Querying data for job/node-test-pull-request/64211/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 55676
From https://github.com/nodejs/node
 * branch                  refs/pull/55676/merge -> FETCH_HEAD
✔  Fetched commits as b3f82febf106..e9de692b3e1e
--------------------------------------------------------------------------------
[main cf4472b6d1] node-api: define version 10
 Author: Gabriel Schulhof <[email protected]>
 Date: Fri Nov 1 07:47:24 2024 -0700
 14 files changed, 39 insertions(+), 58 deletions(-)
[main a189f45485] fixup! TSFN fatal uncaught exception handling
 Author: Gabriel Schulhof <[email protected]>
 Date: Tue Dec 24 18:05:19 2024 -0800
 2 files changed, 7 insertions(+), 7 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
node-api: define version 10

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.

PR-URL: #55676
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>

[detached HEAD 28e90d371e] node-api: define version 10
Author: Gabriel Schulhof <[email protected]>
Date: Fri Nov 1 07:47:24 2024 -0700
14 files changed, 39 insertions(+), 58 deletions(-)
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! TSFN fatal uncaught exception handling

PR-URL: #55676
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>

[detached HEAD 5daeb3497b] fixup! TSFN fatal uncaught exception handling
Author: Gabriel Schulhof <[email protected]>
Date: Tue Dec 24 18:05:19 2024 -0800
2 files changed, 7 insertions(+), 7 deletions(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/12550073787

@gabrielschulhof gabrielschulhof added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 30, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 31, 2024
@nodejs-github-bot nodejs-github-bot merged commit c5aa8b8 into nodejs:main Dec 31, 2024
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c5aa8b8

@gabrielschulhof gabrielschulhof deleted the node-api-10-proposal branch January 1, 2025 07:27
aduh95 pushed a commit that referenced this pull request Jan 2, 2025
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.

PR-URL: #55676
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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
Development

Successfully merging this pull request may close these issues.

4 participants