confusion about NAPI_VERSION, NAPI_EXPERIMENTAL

[DaAitch]

I'm a bit confused about the NAPI versioning, like NAPI_VERSION, NAPI_EXPERIMENTAL macros.

1. deprecation of NAPI_EXPERIMENTAL

With 73fed84 impl has been changed to only use NAPI_VERSION to determine whether features can be used.

1. What not has been adapted are the #endif comments like 73fed84#diff-f546e4cc35b454813e5264fe9c9e6fc8R307.
2. Tests are still defining NAPI_EXPERIMENTAL: this can be removed I think, as well as any other occurrence of NAPI_EXPERIMENTAL in the project
3. npm test --NAPI_VERSION=... should be documented or defined as npm scripts, like npm run test:v2, npm run test:v3, etc.

2. experimental versions

For features which will land with any future napi version currently not defined, we have those #if (NAPI_VERSION > 2147483646) statements. I think it would be easier to read:

#define NAPI_VERSION_EXPERIMENTAL 2147483647

// [...]

#if (NAPI_VERSION == NAPI_VERSION_EXPERIMENTAL)
// ...
#endif

As well as documentation or defining scripts npm run test:experimental

What do you think?

[gabrielschulhof]

@DaAitch NAPI_EXPERIMENTAL comes from N-API. I guess you're right though in that we should not base our decisions on whether NAPI_EXPERIMENTAL is defined, but rather only on what value NAPI_VERSION takes. NAPI_EXPERIMENTAL should only be used to crank up the version number so as to make available all the experimental APIs.

This has to be done in both Node.js core and in node-addon-api though.

[mhdawson]

I'd have to look more closely as I think we need NAPI_EXPERIMENTAL to be defined in the tests as that is what ends up making NAPI_VERSION 2147483646 the default so that without any options npm test runs all of the tests.

I do agree that we should have a constant for NAPI_VERSION_EXPERIMENTAL but I think it makes sense in node core were NAPI_EXPERIMENTAL is defined. Just opened a PR here for that: nodejs/node#25319 . However, we probably need a defined in node-addon-api which sets it if it is not defined so that we cover older versions.

Good point that I missed updating the #endifs

I also definitely agree with us needing to document npm test --NAPI_VERSION=... It's on my long list of TODO's but if you have time to do it I'd be happy :)

[DaAitch]

(also discussed in NAPI team meeting)

• both NAPI_VERSION and NAPI_EXPERIMENTAL are needed
• NAPI_VERSION for using a specific version to compile against (current/default version is 3)
• NAPI_EXPERIMENTAL for using new experimental features not having any version yet

next steps:

• PR src: add NAPI_VERSION_EXPERIMENTAL node#25319 (#define NAPI_VERSION_EXPERIMENTAL 2147483647)
• PR node-addon-api
  • substitute magic number with NAPI_VERSION_EXPERIMENTAL
  • documentation, e.g. describe npm run test --NAPI_VERSION=3
• close issue

anything missing?

[mhdawson]

@DaAitch sounds good to me.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mhdawson]

The referenced PR landed, more recently we've added some doc to the README.md so I think this can be closed. Please let us know if you think that was not the right thing to do.