Skip to content

doc: add inline comment hints on finalizer signatures - #1738

Open
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:finalizer
Open

doc: add inline comment hints on finalizer signatures#1738
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:finalizer

Conversation

@legendecas

@legendecas legendecas commented Jul 13, 2026

Copy link
Copy Markdown
Member

Add inline comment hints on finalizer signatures.

When C++20 is the baseline, we should add C++ concepts to hint about the templated parameters.

@codecov-commenter

codecov-commenter commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.69%. Comparing base (7223518) to head (1a34760).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1738   +/-   ##
=======================================
  Coverage   63.69%   63.69%           
=======================================
  Files           3        3           
  Lines        2063     2063           
  Branches      730      730           
=======================================
  Hits         1314     1314           
  Misses        162      162           
  Partials      587      587           

☔ View full report in Codecov by Harness.
📢 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.

Comment thread napi.h
#endif // NAPI_VERSION > 8

#ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
// FinalizerType must implement `void operator()(Env env)`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have we considered using C++ Concepts for this instead of these human-only annotations?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(Though clearly this doesn't need to block the PR...)

@legendecas legendecas Jul 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it will be a semver-major to bump minimum c++ version to c++20. Right now node-addon-api still supports c++17 as in node v18/v20

"node": "^18 || ^20 || >= 21"

@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Jul 24, 2026
Comment thread napi.h
#endif // NAPI_VERSION > 8

#ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
// FinalizerType must implement `void operator()(Env env)`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the Markdown documentation files also be updated to reflect the implementation signature for the callbacks? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants