Skip to content

Conversation

@vmoroz
Copy link
Member

@vmoroz vmoroz commented May 12, 2023

PR #45715 implemented a new versioning mechanism for Node-API module behavior, and it also introduced a new behavior for napi_ref to accept all value types. Since we are about to create new Node-API version 9, it was decided that the napi_ref for all value types did not have enough "baking period" and it must remain in the experimental status for a while.

In this PR we change the condition when the napi_ref is available for all types - it must be when the module targets NAPI_VERSION_EXPERIMENTAL version. The Node-API documentation is also changed to reflect the experimental status of this napi_refbehavioral change.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels May 12, 2023
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

One minor doc request, otherwise LGTM.

vmoroz and others added 2 commits May 16, 2023 05:30
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

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Co-authored-by: Gabriel Schulhof <[email protected]>
@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label May 18, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/47975
✔  Done loading data for nodejs/node/pull/47975
----------------------------------- PR info ------------------------------------
Title      node-api: make `napi_ref` for all value types to be experimental (#47975)
Author     Vladimir Morozov  (@vmoroz)
Branch     vmoroz:PR/napi_ref_all_values_experimental -> nodejs:main
Labels     c++, node-api, needs-ci
Commits    5
 - node-api: napi_ref for all value types is experimental
 - extract experimental behavior into a paragraph
 - update doc/api/n-api.md
 - fix lint-md issues
 - update doc/api/n-api.md
Committers 2
 - Vladimir Morozov 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/47975
Reviewed-By: Chengzhong Wu 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47975
Reviewed-By: Chengzhong Wu 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - update doc/api/n-api.md
   ℹ  This PR was created on Fri, 12 May 2023 14:21:41 GMT
   ✔  Approvals: 2
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/47975#pullrequestreview-1428929792
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/47975#pullrequestreview-1429103385
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-05-16T17:58:27Z: https://ci.nodejs.org/job/node-test-pull-request/51830/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - update doc/api/n-api.md
- Querying data for job/node-test-pull-request/51830/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/5014193886

mhdawson pushed a commit that referenced this pull request May 18, 2023
PR-URL: #47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed as 0063669

@mhdawson mhdawson closed this May 18, 2023
fasenderos pushed a commit to fasenderos/node that referenced this pull request May 22, 2023
PR-URL: nodejs#47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@vmoroz vmoroz deleted the PR/napi_ref_all_values_experimental branch May 23, 2023 18:09
targos pushed a commit that referenced this pull request May 30, 2023
PR-URL: #47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Jun 4, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47975
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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants