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.

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

[v10.x] src: yield empty maybes for failed AsyncWrap::MakeCallback calls #22505

Closed
wants to merge 2 commits into from
Closed

[v10.x] src: yield empty maybes for failed AsyncWrap::MakeCallback calls #22505

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Clean cherry-pick of #22078 + fixup code

@addaleax
src: yield empty maybes for failed AsyncWrap::MakeCallback calls
4f5b0f9 
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: nodejs#22078
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@addaleax
fixup! src: yield empty maybes for failed AsyncWrap::MakeCallback calls
5d42a91
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/661/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v10.x labels Aug 24, 2018
targos
targos approved these changes Aug 25, 2018
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Thank you!

@addaleax
Copy link
Member Author

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

@targos
Copy link
Member

targos commented Aug 28, 2018 •
edited by addaleax
Loading

New CI: https://ci.nodejs.org/job/node-test-pull-request/16831/ (✔️)

@addaleax
Copy link
Member Author

addaleax commented Aug 28, 2018 •
edited
Loading

Landed in 63f78fa

@addaleax addaleax closed this Aug 28, 2018
@addaleax addaleax deleted the backport-22078-v10.x branch August 28, 2018 19:43
addaleax added a commit that referenced this pull request Aug 28, 2018
@addaleax
src: yield empty maybes for failed AsyncWrap::MakeCallback calls
63f78fa 
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Backport-PR-URL: #22505
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
@addaleax @targos
src: yield empty maybes for failed AsyncWrap::MakeCallback calls
198cf41 
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Backport-PR-URL: #22505
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
@addaleax @targos
src: yield empty maybes for failed AsyncWrap::MakeCallback calls
727187d 
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Backport-PR-URL: #22505
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@addaleax @nodejs-github-bot @targos