Skip to content

src: Reset error struct if error code is napi_ok #40552

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
wants to merge 1 commit into from
Closed

src: Reset error struct if error code is napi_ok #40552

wants to merge 1 commit into from

Conversation

JckXia
Copy link
Member

@JckXia JckXia commented Oct 21, 2021

Resetting last_error struct inside the env instance if the error code is napi_ok to make sure that the error code is consistent with any other meta data inside the struct.
ref: nodejs/node-addon-api#1089

@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. labels Oct 21, 2021
RaisinTen
RaisinTen approved these changes Oct 23, 2021
View reviewed changes
Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM. @mhdawson could you take a look at this too?

@RaisinTen RaisinTen requested a review from mhdawson October 23, 2021 15:31
@JckXia JckXia force-pushed the reset-err-info-if-napi-ok branch from 61c473e to 2bdcfd5 Compare October 24, 2021 17:15
mhdawson
mhdawson approved these changes Oct 25, 2021
View reviewed changes
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.

LGTM

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions bot mentioned this pull request Oct 26, 2021
Open
25 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Oct 27, 2021
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Oct 30, 2021
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Nov 4, 2021
Open
Open
Open
Open
Open
Open
@JckXia JckXia force-pushed the reset-err-info-if-napi-ok branch from 2bdcfd5 to ad97f6e Compare November 14, 2021 22:40
@JckXia
Copy link
Member Author

JckXia commented Nov 15, 2021

@mhdawson It looks like the CI has passed after a rebase.

@mhdawson mhdawson added the node-api Issues and PRs related to the Node-API. label Nov 15, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member

@JckXia I needed to kick off a CI, the passing checks are the ones done through actions.

A commend with the new CI should be added to the issue soon and lets keep our fingers crossed that it passes this time.

@github-actions github-actions bot mentioned this pull request Nov 16, 2021
Open
40 tasks
@JckXia
Copy link
Member Author

JckXia commented Nov 16, 2021

@mhdawson Hmm it looks like the CI failed again. Will do more digging.

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Nov 17, 2021
Open
39 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member

@JckXia I think it's still CI flakyness based on the failures, not your PR. I've resumed once more and hopefully we'll get to green.

@JckXia
Copy link
Member Author

JckXia commented Nov 17, 2021

Thank you @mhdawson!

@mhdawson
Copy link
Member

It's greeeeen :)

@mhdawson
Copy link
Member

Landed in 86e976f

@mhdawson mhdawson closed this Nov 17, 2021
mhdawson pushed a commit that referenced this pull request Nov 17, 2021
@JckXia @mhdawson
src: reset error struct if error code is napi_ok
86e976f 
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@mhdawson
Copy link
Member

@JckXia thanks for your patience and work on this one:)

@JckXia
Copy link
Member Author

JckXia commented Nov 17, 2021

@mhdawson Thank you so much! :)

This was referenced Nov 18, 2021
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 21, 2021
@JckXia @targos
src: reset error struct if error code is napi_ok
c2259c9 
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@targos targos mentioned this pull request Nov 26, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@JckXia @danielleadams
src: reset error struct if error code is napi_ok
6532063 
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@JckXia @danielleadams
src: reset error struct if error code is napi_ok
7237bcc 
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
@JckXia JckXia deleted the reset-err-info-if-napi-ok branch February 11, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@mhdawson mhdawson mhdawson approved these changes

@juanarbol juanarbol juanarbol approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

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 the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JckXia @nodejs-github-bot @mhdawson @jasnell @gabrielschulhof @juanarbol @RaisinTen