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: disambiguate napi_add_finalizer by legendecas · Pull Request #45401 · nodejs/node

Conversation

@legendecas

@legendecas legendecas commented Nov 10, 2022

Copy link
Copy Markdown
Member

napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2022

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 Nov 10, 2022
VoltrexKeyva
VoltrexKeyva reviewed Nov 10, 2022
View reviewed changes
Comment thread doc/api/n-api.md Resolved

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@legendecas
legendecas force-pushed the node-api/add-finalizer branch from 77df31f to c4d496b Compare November 10, 2022 16:48
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
mhdawson
mhdawson reviewed Dec 2, 2022
View reviewed changes
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson reviewed Dec 2, 2022
View reviewed changes
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson approved these changes Dec 2, 2022
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.

One small suggestion, but otherwise looks good to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@legendecas
legendecas force-pushed the node-api/add-finalizer branch 2 times, most recently from eaae589 to ce5f9ab Compare December 5, 2022 16:17
@legendecas legendecas 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 Dec 5, 2022
@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 Dec 5, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 5, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Dec 7, 2022
CI Reliability 2022-12-07 nodejs/reliability#450
Open
CI Reliability 2022-12-08 nodejs/reliability#451
Open
CI Reliability 2022-12-09 nodejs/reliability#452
Open
CI Reliability 2022-12-10 nodejs/reliability#453
Open
@legendecas legendecas 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 Dec 19, 2022
@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 Dec 19, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 19, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

mhdawson commented Dec 19, 2022

Copy link
Copy Markdown
Member

@legendecas looks like this needs a rebase now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Dec 20, 2022
CI Reliability 2022-12-20 nodejs/reliability#463
Open
14 tasks
@legendecas
node-api: disambiguate napi_add_finalizer
b04b070 
napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.
@legendecas
legendecas force-pushed the node-api/add-finalizer branch from ce5f9ab to b04b070 Compare December 20, 2022 03:01
@legendecas legendecas 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 Dec 20, 2022
@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 Dec 20, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 20, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 20, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

legendecas commented Dec 20, 2022

Copy link
Copy Markdown
Member Author

@mhdawson rebased. Would you mind taking a look again? thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Dec 21, 2022
CI Reliability 2022-12-21 nodejs/reliability#464
Open
CI Reliability 2022-12-22 nodejs/reliability#465
Open
CI Reliability 2022-12-23 nodejs/reliability#466
Open
@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 Jan 3, 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 Jan 3, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 3, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas added a commit that referenced this pull request Jan 4, 2023
@legendecas
node-api: disambiguate napi_add_finalizer
198d203 
napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.

PR-URL: #45401
Reviewed-By: Michael Dawson <[email protected]>
@legendecas

legendecas commented Jan 4, 2023

Copy link
Copy Markdown
Member Author

Landed in 198d203
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas legendecas closed this Jan 4, 2023
@legendecas
legendecas deleted the node-api/add-finalizer branch January 4, 2023 10:57
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
@legendecas @RafaelGSS
node-api: disambiguate napi_add_finalizer
aa50f8a 
napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.

PR-URL: nodejs#45401
Reviewed-By: Michael Dawson <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@legendecas @RafaelGSS
node-api: disambiguate napi_add_finalizer
bd98e5b 
napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.

PR-URL: #45401
Reviewed-By: Michael Dawson <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
Merged
toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Jan 21, 2023
@toyobayashi
disambiguate napi_add_finalizer (nodejs/node#45401)
adbc3d0
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@legendecas @juanarbol
node-api: disambiguate napi_add_finalizer
c0e7738 
napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.

PR-URL: #45401
Reviewed-By: Michael Dawson <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
V18.14.0 proposal #46396
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@legendecas @juanarbol
node-api: disambiguate napi_add_finalizer
2ab35cf 
napi_add_finalizer doesn't support any operations related to
napi_wrap. Remove the ambiguous statements in the doc about
napi_wrap and avoid reusing the v8impl::Wrap call.

PR-URL: #45401
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments
@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++. 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.

4 participants

@legendecas @nodejs-github-bot @mhdawson @VoltrexKeyva
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.