node-api: rename internal NAPI_VERSION definition
The NAPI_VERSION defined in node_version.h has a different meaning than the one defined in the js_native_api.h. The one from node_version.h is identical to napi_get_version(), indicating the highest Node-API version supported by the Node.js runtime. While the NAPI_VERSION defined in js_native_api.h indicates the Node-API version required by the addon and should be lower than or equal to the NAPI_VERSION of the Node.js runtime.
Rename the one defined in node_version.h to NODE_API_MODULE_API_VERSION (aligning with NODE_API_DEFAULT_MODULE_API_VERSION). This avoids unexpected duplicated definitions of NAPI_VERSION when both node_version.h and js_native_api.h are included.
Fixes https://github.com/nodejs/node/issues/48310
Review requested:
- [ ] @nodejs/node-api
@legendecas , could we use a different name instead of NODE_API_MODULE_API_VERSION?
For example, NODE_API_SUPPORTED_VERSION, NODE_API_RUNTIME_VERSION, or NODE_API_IMPLEMENTATION_VERSION.
I.e. any name that would indicate that this is a version for the Node-API implementation rather than the Node-API version used by a module.
The NODE_API_DEFAULT_MODULE_API_VERSION was created to indicate a default version of Node-API used by a module. Thus, it has the MODULE word inside. If we use MODULE in the name of the macro that indicates the currently implemented Node-API version, then it will sound a bit confusing.
Ideally, we must have two different macros to indicate the lowest and highest supported Node-API versions for the given Node.js executable. E.g. NODE_API_MAX_SUPPORTED_VERSION and NODE_API_MIN_SUPPORTED_VERSION. This way we will be able to deprecate Node-API versions which we cannot support anymore.
CI: https://ci.nodejs.org/job/node-test-pull-request/52352/
I agree we should find a name that more clearly communicates what the value means.
NODE_API_MAX_SUPPORTED_VERSION makes sense to me. I don't think we min the MIN version yet so I'd leave that out for now but using NODE_API_MAX_SUPPORTED_VERSION as the naming gives us that option.
Thanks for the suggestion!
Renamed to NODE_API_SUPPORTED_VERSION_MAX and NODE_API_SUPPORTED_VERSION_MIN and added min version check in get_node_api_context_register_func to disallow arbitrary version that is lower than NODE_API_DEFAULT_MODULE_API_VERSION being registered with node_api_module_get_api_version callback.
CI: https://ci.nodejs.org/job/node-test-pull-request/52447/
CI: https://ci.nodejs.org/job/node-test-pull-request/52475/
CI: https://ci.nodejs.org/job/node-test-pull-request/52544/
Landed in 6ffacbf0f9e4