src: restore ability to run under NAPI_EXPERIMENTAL

[gabrielschulhof]

Since we made the default for Node.js core finalizers synchronous for users running with NAPI_EXPERIMENTAL and introduced env->CheckGCAccess() in Node.js core, we must now defer all finalizers in node-addon-api, because our users likely make non-gc-safe Node-API calls from existing finalizers. To that end,

• Use the NAPI_VERSION environment variable to detect whether NAPI_EXPERIMENTAL should be on, and add it to the defines if NAPI_VERSION is set to NAPI_VERSION_EXPERIMENTAL, i.e. 2147483647.
• When building with NAPI_EXPERIMENTAL,
  • render all finalizers asynchronous, and
  • expect napi_cannot_run_js instead of napi_exception_pending.

BEGIN_COMMIT_OVERRIDE
fix: restore ability to run under NAPI_EXPERIMENTAL (#1409)
END_COMMIT_OVERRIDE

[mhdawson]

I might have been thinking of another change but I thought we had made it opt-in with another define? @legendecas is that right?

[legendecas]

because our users likely make non-gc-safe Node-API calls from existing finalizers

I'm afraid that I didn't understand this assumption. Is this assumption unique to node-addon-api, not the node-api c layer?

[gabrielschulhof]

Core status of needed changes:

branch	backport complete	released
v22.x	✅	✅ (v22.0.0)
v20.x	✅	✅ (20.13.0)
v18.x	✅	✅ (v18.20.3)

[KevinEady]

Hi @gabrielschulhof ,

Looks like this is becoming of more importance. I can take a look at continuing this work? Judging by the latest CI failure, looks like the Napi::External<> object needs to implement a similar approach.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.89%. Comparing base (57ba3df) to head (2bfdaac).
Report is 3 commits behind head on main.

Files	Patch %	Lines
napi-inl.h	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
+ Coverage   64.71%   64.89%   +0.17%     
==========================================
  Files           3        3              
  Lines        1981     1991      +10     
  Branches      687      687              
==========================================
+ Hits         1282     1292      +10     
  Misses        138      138              
  Partials      561      561              

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

[gabrielschulhof]

v18.x fails because v18.20.3, which is the one where the thread-safe function revert-to-napi_env landed, is not yet available via github actions.

[gabrielschulhof]

Removed v21.x from the matrix because the needed fixes were never backported to it and because it will be discontinued soon, per https://github.com/nodejs/Release/raw/main/schedule.svg

[KevinEady]

See comment for discussion.

[KevinEady]

See comments.

[KevinEady]

LGTM!

[gabrielschulhof]

Version	Job	Status
18.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/8927/	✅
20.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/8928/	✅
22.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/8929/	✅

[mhdawson]

LGTM

[KevinEady]

@gabrielschulhof,

@vmoroz noticed on my PR that i am not testing experimental on Windows, which is also the case on this PR. Should Windows experimental also be tested? ref: #1514 (comment)

[gabrielschulhof]

@KevinEady let's land this and add experimental on Windows in a different PR. I filed an issue so we don't forget.

[mhdawson]

Based on discussion in the team meeting today, removed the SemVer-major lable as we believe it should be patch.