Review requested:

• @nodejs/node-api

Should we consider a bug fix as a breaking change? I don't think we should introduce a new workaround for bug fixes if the bug fix is not leading to fatal errors for legit use cases.

Should we consider a bug fix as a breaking change? I don't think we should introduce a new workaround for bug fixes if the bug fix is not leading to fatal errors for legit use cases.

In this PR we are introducing a new mechanism based on features that allows each module developer to choose the behavior they want:

• In case if developer did not recompile addon code, then it is going to continue to behave the same way as it was before because the nm_features field in the napi_module is going to be initialized to 0, or whatever value it used to have before (in future).
• As soon as the addon is recompiled, it will acquire the new behavior only if it adopts a new version of Node-API.
• If a specific new feature is breaking the addon's code in the new Node-API version, developers can choose to exclude that feature just for that addon.
• The cool part is that using the node_api_add_features and node_api_remove_features allows developers to control the new behavior for a subset of code. E.g. in the test_object.c we check the old the new behavior by turning node_api_features_use_configurable_key_filter feature "off" and "on".

Unlike the normal breaking changes that we had before which usually affect behavior of all addons and must be controlled by a node.js flag, this mechanism allows to:

• control the new behavior per addon and be adopted only after it is recompiled;
• backport fixes to previous versions of Node.js without causing widespread code breaks because new features are "on" by default only in the newer versions of Node-API, and addons compiled against the old version will not "see" them. The addons that require the new behavior in older versions can force to turn it "on" after it is backported.

I wonder if it would a better mechanism to adopt our changes in PR #36510 and #42208.

I mean, why shouldn't we just fix the problem? This is not working as expected and fixing it won't introduce any fatal errors. I think fixing it will be a more straightforward option.

I mean, why shouldn't we just fix the problem? This is not working as expected and fixing it won't introduce any fatal errors. I think fixing it will be a more straightforward option.

I agree, but when we discussed this issue a few months ago, the agreement was that it is a behavioral change that may affect existing code. Thus, I thought it would be a good example to introduce the support of features.
For that specific issue if we agree that we do not need to use the features, then I can remove them from this PR and introduce them in another PR.

Per our discussion today in the Node-API meeting I am going to split up this PR into two parts: the bug fix and the implementation of features.

Though, I can probably do it for the current object only without including prototypes.

Is that something you'd like to do before we land this or possibly as as follow on. I see @legendecas's comment is not blocking but don't want to land if you want to update before we do that.

Though, I can probably do it for the current object only without including prototypes.

Is that something you'd like to do before we land this or possibly as follow on. I see @legendecas's comment is not blocking but don't want to land if you want to update before we do that.

@mhdawson, I have added tests for own non-enumerable properties to test writable and configurable flags in isolation.

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

- Loading data for nodejs/node/pull/42463
✔  Done loading data for nodejs/node/pull/42463
----------------------------------- PR info ------------------------------------
Title      node-api: fix napi_get_all_property_names use of napi_key_configurable filter (#42463)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     vmoroz:PR/FixGetAllPropertyNames -> nodejs:master
Labels     c++, lib / src, node-api, needs-ci, commit-queue-squash
Commits    4
 - node-api: fix napi_get_all_property_names
 - remove support for features
 - Merge branch 'master' into PR/FixGetAllPropertyNames
 - add tests for non-enumerable properties
Committers 1
 - Vladimir Morozov 
PR-URL: https://github.com/nodejs/node/pull/42463
Reviewed-By: Michael Dawson 
Reviewed-By: Chengzhong Wu 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42463
Reviewed-By: Michael Dawson 
Reviewed-By: Chengzhong Wu 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Mar 2022 06:12:10 GMT
   ✔  Approvals: 3
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/42463#pullrequestreview-939948462
   ✔  - Chengzhong Wu (@legendecas): https://github.com/nodejs/node/pull/42463#pullrequestreview-928378619
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42463#pullrequestreview-941150971
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-04-12T18:03:32Z: https://ci.nodejs.org/job/node-test-pull-request/43463/
- Querying data for job/node-test-pull-request/43463/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 42463
From https://github.com/nodejs/node
 * branch                  refs/pull/42463/merge -> FETCH_HEAD
✔  Fetched commits as 3026ca0bf2d6..1d071f0f1c58
--------------------------------------------------------------------------------
Auto-merging src/node_api.h
[master 822ca46330] node-api: fix napi_get_all_property_names
 Author: Vladimir Morozov 
 Date: Thu Mar 24 22:55:39 2022 -0700
 10 files changed, 245 insertions(+), 12 deletions(-)
Auto-merging src/node_api.h
error: commit da279a1982b27fd85605f7d8a698c94934df1ce0 is a merge but no -m option was given.
fatal: cherry-pick failed
[master 90936a3715] remove support for features
 Author: Vladimir Morozov 
 Date: Wed Mar 30 07:39:52 2022 -0700
 10 files changed, 11 insertions(+), 156 deletions(-)
   ✖  Failed to apply patches

@vmoroz could you rebase and squash the commits? I tried manually and with the commit queue and it won't easily squash.

@vmoroz could you rebase and squash the commits? I tried manually and with the commit queue and it won't easily squash.

Let me try.

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

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

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

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

Landed in 6b968fd

@vmoroz thanks for all your work/patience in getting this landed.