node icon indicating copy to clipboard operation
node copied to clipboard

node-api: rename internal NAPI_VERSION definition

Open legendecas opened this issue 2 years ago • 5 comments

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

legendecas avatar Jun 20 '23 02:06 legendecas

Review requested:

  • [ ] @nodejs/node-api

nodejs-github-bot avatar Jun 20 '23 02:06 nodejs-github-bot

@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.

vmoroz avatar Jun 20 '23 16:06 vmoroz

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

nodejs-github-bot avatar Jun 22 '23 16:06 nodejs-github-bot

I agree we should find a name that more clearly communicates what the value means.

mhdawson avatar Jun 23 '23 15:06 mhdawson

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.

mhdawson avatar Jun 23 '23 15:06 mhdawson

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.

legendecas avatar Jun 25 '23 07:06 legendecas

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

nodejs-github-bot avatar Jun 25 '23 07:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 26 '23 01:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 28 '23 20:06 nodejs-github-bot

Landed in 6ffacbf0f9e4

mhdawson avatar Jun 28 '23 21:06 mhdawson