confusion about NAPI_VERSION, NAPI_EXPERIMENTAL #421
Description
I'm a bit confused about the NAPI versioning, like NAPI_VERSION, NAPI_EXPERIMENTAL macros.
1. deprecation of NAPI_EXPERIMENTAL
NAPI_EXPERIMENTALWith 73fed84 impl has been changed to only use NAPI_VERSION to determine whether features can be used.
- What not has been adapted are the
#endifcomments like 73fed84#diff-f546e4cc35b454813e5264fe9c9e6fc8R307. Tests are still definingNAPI_EXPERIMENTAL: this can be removed I think, as well as any other occurrence ofNAPI_EXPERIMENTALin the projectnpm test --NAPI_VERSION=...should be documented or defined as npm scripts, likenpm 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)
// ...
#endifAs well as documentation or defining scripts npm run test:experimental
What do you think?
Activity
gabrielschulhof commentedon Dec 29, 2018
@DaAitch
NAPI_EXPERIMENTALcomes from N-API. I guess you're right though in that we should not base our decisions on whetherNAPI_EXPERIMENTALis defined, but rather only on what valueNAPI_VERSIONtakes.NAPI_EXPERIMENTALshould 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.
src: add NAPI_VERSION_EXPERIMENTAL
mhdawson commentedon Jan 2, 2019
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 commentedon Jan 3, 2019
(also discussed in NAPI team meeting)
next steps:
#define NAPI_VERSION_EXPERIMENTAL 2147483647)npm run test --NAPI_VERSION=3anything missing?
mhdawson commentedon Jan 4, 2019
@DaAitch sounds good to me.
src: add NAPI_VERSION_EXPERIMENTAL
src: add NAPI_VERSION_EXPERIMENTAL
src: add NAPI_VERSION_EXPERIMENTAL
src: add NAPI_VERSION_EXPERIMENTAL
github-actions commentedon Dec 17, 2020
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 commentedon Dec 17, 2020
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.