Skip to content

Conversation

legendecas
Copy link
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.

@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 Nov 10, 2022
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.

One small suggestion, but otherwise looks good to me.

@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. label Dec 5, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

@legendecas looks like this needs a rebase now.

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. label Dec 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

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

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

legendecas added a commit that referenced this pull request Jan 4, 2023
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
Copy link
Member Author

Landed in 198d203

@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
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
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
toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Jan 21, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
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
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
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
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 the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants