Skip to content

Conversation

@KevinEady
Copy link
Contributor

@KevinEadyKevinEady commented Oct 18, 2024

Add new compile-time define (NODE_ADDON_API_CPP_EXCEPTIONS_ALL) to enable catching all exceptions. This must be used in conjunction withNODE_ADDON_API_CPP_EXCEPTIONS.

Rename the NAPI_ prefixed exception preprocessor definitions (NAPI_CPP_EXCEPTIONS and NAPI_DISABLE_CPP_EXCEPTIONS) to be prefixed with our standard NODE_ADDON_API_ prefix in a backwards-compatible way.

Fixes: #1555

@KevinEady
Copy link
ContributorAuthor

I'd like to discuss the naming of the new define NAPI_CPP_EXCEPTIONS_ALL ... to go with convention it should be NODE_ADDON_API_CPP_EXCEPTIONS_ALL.

We can use NODE_ADDON_API_CPP_EXCEPTIONS_ALL and introduce NODE_ADDON_API_CPP_EXCEPTIONS as a (backwards-compatible) replacement for NAPI_CPP_EXCEPTIONS_ALL ?

@KevinEady
Copy link
ContributorAuthor

We discuss in the 18 Oct Node-API meeting:

  • Use the NODE_ADDON_API_ prefix on the defines
  • Should the "catch all" be the new default?
    • What about some best practices doc? We also mentioned this with the requiring basic finalizers option

Copy link
Member

@NickNasoNickNaso left a comment

Choose a reason for hiding this comment

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

Is this branch in sync with the main?

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 66.29%. Comparing base (98aae33) to head (df4e6b1).
Report is 28 commits behind head on main.

Files with missing linesPatch %Lines
napi-inl.h85.00%1 Missing and 5 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #1593 +/- ## ========================================== - Coverage 66.35% 66.29% -0.07%  ========================================== Files 3 3 Lines 2143 2154 +11 Branches 703 708 +5 ========================================== + Hits 1422 1428 +6  Misses 150 150 - Partials 571 576 +5 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KevinEadyKevinEady marked this pull request as ready for review November 7, 2024 23:35
@KevinEady
Copy link
ContributorAuthor

Hi @vmoroz@mhdawson@legendecas@gabrielschulhof , I believe this is ready.

Copy link
Member

@legendecaslegendecas left a comment

Choose a reason for hiding this comment

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

Looks good to me! A minor suggestion...

Co-authored-by: Chengzhong Wu <[email protected]>
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

@mhdawsonmhdawson merged commit c679f6f into nodejs:mainNov 15, 2024
54 checks passed
@github-actionsgithub-actionsbot mentioned this pull request Nov 15, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

std::exception catching once more

5 participants

@KevinEady@codecov-commenter@NickNaso@legendecas@mhdawson