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: make napi_ref for all value types to be experimental #47975

Closed

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.

doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
vmoroz and others added 2 commits May 16, 2023 05:30
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
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

doc/api/n-api.md Outdated Show resolved Hide resolved
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@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 <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@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 <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@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 <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@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 <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47975
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
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