GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — node-api: make `napi_ref` for all value types to be experimental by vmoroz · Pull Request #47975 · nodejs/node

Conversation

@vmoroz

@vmoroz vmoroz commented May 12, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@vmoroz
node-api: napi_ref for all value types is experimental
44dba47
@nodejs-github-bot

nodejs-github-bot commented May 12, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/node-api
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 Node-API. labels May 12, 2023
gabrielschulhof
gabrielschulhof reviewed May 12, 2023
View reviewed changes

@gabrielschulhof gabrielschulhof left a comment

Copy link
Copy Markdown
Contributor

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.

One minor doc request, otherwise LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof mentioned this pull request May 12, 2023
doc: document node-api version process #47972
Merged
toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request May 13, 2023
@toyobayashi
napi_ref for all value types is experimental (nodejs/node#47975)
dc4edc8
@vmoroz
extract experimental behavior into a paragraph
326c6b3
gabrielschulhof
gabrielschulhof reviewed May 16, 2023
View reviewed changes
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
vmoroz and others added 2 commits May 16, 2023 05:30
@vmoroz @gabrielschulhof
update doc/api/n-api.md
9476001 
Co-authored-by: Gabriel Schulhof <[email protected]>
@vmoroz
fix lint-md issues
d4a6e47
legendecas
legendecas approved these changes May 16, 2023
View reviewed changes
mhdawson
mhdawson approved these changes May 16, 2023
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 16, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 16, 2023
@nodejs-github-bot

nodejs-github-bot commented May 16, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51830/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gabrielschulhof
gabrielschulhof reviewed May 16, 2023
View reviewed changes
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@vmoroz @gabrielschulhof
update doc/api/n-api.md
05c689f 
Co-authored-by: Gabriel Schulhof <[email protected]>
@legendecas legendecas added the commit-queue PRs queued for automated landing through the Commit Queue. label May 18, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels May 18, 2023
@nodejs-github-bot

nodejs-github-bot commented May 18, 2023

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson pushed a commit that referenced this pull request May 18, 2023
@vmoroz @mhdawson
node-api: napi_ref on all types is experimental
0063669 
PR-URL: #47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson

mhdawson commented May 18, 2023

Copy link
Copy Markdown
Member

Landed as 0063669
vmoroz reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson mhdawson closed this May 18, 2023
@vmoroz
vmoroz deleted the PR/napi_ref_all_values_experimental branch May 23, 2023 18:09
@legendecas legendecas mentioned this pull request May 24, 2023
node-api: define version 9 #48151
Merged
targos pushed a commit that referenced this pull request May 30, 2023
@vmoroz @targos
node-api: napi_ref on all types is experimental
eafe0c3 
PR-URL: #47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Jun 4, 2023
v20.3.0 release proposal #48332
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@vmoroz @danielleadams
node-api: napi_ref on all types is experimental
f8f70d8 
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
@vmoroz @MoLow
node-api: napi_ref on all types is experimental
4b280d5 
PR-URL: nodejs#47975
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
v18.17.0 release proposal #48694
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabrielschulhof gabrielschulhof gabrielschulhof left review comments
@legendecas legendecas legendecas approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@vmoroz @nodejs-github-bot @mhdawson @gabrielschulhof @legendecas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.