Skip to content

[v14.x] Backport 33570 and 33508 to v14.x#33817

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.

Sign up for GitHub

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

Jump to bottom
Closed
gabrielschulhof wants to merge 2 commits into from
Closed

[v14.x] Backport 33570 and 33508 to v14.x #33817

gabrielschulhof wants to merge 2 commits into from

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhofgabrielschulhof commented Jun 10, 2020

#33508 cannot be backported directly, but must be preceded with a backport of #33570.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Gabriel Schulhofand others added 2 commits June 9, 2020 16:23
n-api: remove napi_env::CallIntoModuleThrow
a405569
Give `napi_env::CallIntoModule` the thrower used by `CallIntoModuleThrow` as its default second argument. That way we do not need two different methods on `napi_env` for calling into the addon. PR-URL: nodejs#33570 Signed-off-by: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
n-api: ensure scope present for finalization
b055a52
Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#33508 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. v14.x labels Jun 10, 2020
@gabrielschulhofgabrielschulhof changed the title Backport 33570 and 33508 to v14.x[v14.x] Backport 33570 and 33508 to v14.xJun 10, 2020
mhdawson
mhdawson approved these changes Jun 11, 2020
View reviewed changes
Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 12, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31846/

@codebytere
Copy link
Member

codebytere commented Jun 27, 2020

@gabrielschulhof this fails to apply with git node land --backport 33817 on latest staging 🤔

Applying: n-api: remove `napi_env::CallIntoModuleThrow` error: patch failed: src/js_native_api_v8.cc:267 error: src/js_native_api_v8.cc: patch does not apply error: patch failed: src/js_native_api_v8.h:82 error: src/js_native_api_v8.h: patch does not apply error: patch failed: src/node_api.cc:57 error: src/node_api.cc: patch does not apply Patch failed at 0001 n-api: remove `napi_env::CallIntoModuleThrow` hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". -------------------------------------------------------------------------------- ? The normal `git am` failed. Do you want to retry with 3-way merge? Yes Applying: n-api: remove `napi_env::CallIntoModuleThrow` Using index info to reconstruct a base tree... M src/js_native_api_v8.cc M src/js_native_api_v8.h M src/node_api.cc Falling back to patching base and 3-way merge... Auto-merging src/node_api.cc Auto-merging src/js_native_api_v8.cc CONFLICT (content): Merge conflict in src/js_native_api_v8.cc Recorded preimage for 'src/js_native_api_v8.cc' error: Failed to merge in the changes. Patch failed at 0001 n-api: remove `napi_env::CallIntoModuleThrow` hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

@gabrielschulhof
Copy link
ContributorAuthor

gabrielschulhof commented Jun 29, 2020

@codebytere it looks like these two have already landed as 524daf8 and e83642f.

@gabrielschulhofgabrielschulhof deleted the backport-33508-to-v14.x branch July 10, 2020 18:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecaslegendecaslegendecas approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

@addaleaxaddaleaxAwaiting requested review from addaleax

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gabrielschulhof@nodejs-github-bot@codebytere@legendecas@mhdawson