node-api: remove deprecated attribute from napi_module_register

[vmoroz]

This PR targets to addresses the issue #56153 by removing the [[deprecated]] attribute from the napi_module_register function.

The code described in the issue does not use the napi_module_register function as it was intended from a module shared library. Instead, it uses it to register modules for embedding scenarios. While it was never the intended use, we currently do not have a better approach to register modules before Environment is created until we complete the new embedding API - PR #54660.

We discussed the issue in the Node-API meeting on Dec 6th 2024, and decided that the quickest and the least intrusive way to address it is to remove the [[deprecated]] attribute from the napi_module_register function for now.

[nodejs-github-bot]

Review requested:

• @nodejs/node-api

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (4211ab5) to head (0b18610).
Report is 107 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56162      +/-   ##
==========================================
- Coverage   88.00%   88.00%   -0.01%     
==========================================
  Files         656      656              
  Lines      189103   189103              
  Branches    35999    36004       +5     
==========================================
- Hits       166421   166413       -8     
+ Misses      15860    15856       -4     
- Partials     6822     6834      +12     

see 32 files with indirect coverage changes

[Yeaseen]

IMO, the comments should also be removed, as they might create confusion. @vmoroz

// Deprecated. Replaced by symbol-based registration defined by NAPI_MODULE
// and NAPI_MODULE_INIT macros.

[vmoroz]

IMO, the comments should also be removed, as they might create confusion. @vmoroz

// Deprecated. Replaced by symbol-based registration defined by NAPI_MODULE
// and NAPI_MODULE_INIT macros.

No, the method is deprecated. We do not recommend to use it in future for any new Node-API modules.

In the referenced issue #56153 it is used for embedded scenarios and we cannot offer any alternative to its behaviour in such case until the new C-based embedding API is completed. After that we may try to add the [[deprecated]] attribute again.

The main reason to remove the [[deprecated]] for now is that many production secure supply chain verifiers will raise an error for using deprecated APIs as they consider it as a security risk. In this case there is no security risk for using the napi_module_register. Its only limitation is that it only supports Node-API version 8. This method does not allow using Node-API versions after that.

[Yeaseen]

Thanks for the clarification! @vmoroz

[mhdawson]

LGTM

[vmoroz]

I would like to go back to review the napi_module_register role for the embedding scenarios in future.
We should also write better docs about the Node-API module initialization.
Though, I want to keep the scope of this PR to be small and just to remove the [[deprecated]] attribute to unblock scenarios where this method is still used.

Considering that we want to discuss the future of the napi_module_register with the whole Node-API team, we will do it in the beginning of 2025 after the holidays.

[nodejs-github-bot]

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

[viferga]

Please keep backwards compatibility. You must consider having a C unstable API is a huge headache for embedders. Normally C APIs like CPython or CRuby.. remain stable during years. Even multiple libraries are ABI stable during a lot of years.

Also keep backwards compatibility for APIs like node::Start. If you keep changing this over and over you force projects like mine to rewrite big parts of the project each two or four versions of NodeJS.

This kind of behavior is acceptable in JavaScript but it is not in C/C++. At least keep them for backward compatibility. It won't hurt anybody, and deprecation will.

[vmoroz]

Also keep backwards compatibility for APIs like node::Start. If you keep changing this over and over you force projects like mine to rewrite big parts of the project each two or four versions of NodeJS.

The node::Start is not a part of ABI stable Node-API. It is part of the Node.js embedding API which is not ABI stable. AFAIK, all Node.js C++ APIs are not ABI stable and are changed when needed. Our Node-API team do not have the ownership over these APIs and I cannot say much about it. We work on the new C-based embedding API which we plan to make ABI stable at some point. The new C-based API has function node_embedding_run_main that targets to replace the node::Start for simple CLI cases with options for additional customizations.

As for the ABI-stable Node-APIs, when the API evolves, we can deprecate some old APIs as long as any previously compiled modules continue to work without issues. The newly compiled code will produce a warning for the deprecated functions and supposed to be changed to use the new recommended APIs. It is a normal process that you often find in other big systems.

Why did we deprecate the napi_module_register API?
The napi_module_register is supposed to be called by C++ static variable initialization while DLL/SO is being loaded. The process was quite brittle and we had issues with modules written in non-C++ languages such as Rust or C. To address the needs for other languages, a long time ago an alternative module registration mechanism was added. Node.js loads DLL and then calls an initialization function with a well-known name. So, rather than maintaining two different module initialization methods we deprecated the C++-only one based on napi_module_register in favor of the another one that works for all languages. The main part of the change was to change the module registration macros to use the well-defined name.

It also coincided with adding ability to specify Node-API version used by modules. All modules that do not provide a well defined functions to return Node-API version are going to use the Node-API version 8 - the version we had when we implemented the change.

The change went painless for all developers who used the recommended module registration macros. Unfortunately or fortunately, the [[deprecated]] attribute helped us to discover a use case for this API as you described in the issue #56153. I was not aware about using napi_module_register for embedded scenarios since we have the C++ node::AddLinkedBinding for such cases. The new node::AddLinkedBinding overload supports calling the module initialization function and receives the Node-API version.

The only difference between the node::AddLinkedBinding and napi_module_register is how the modules are accessed from JavaScript. We are going to discuss the future of napi_module_register in our Node-API meetings. For now we will just remove the [[deprecated]] attribute in hope that it should address the immediate issue that you see.

[legendecas]

LGTM. The original intention is not removing this method for ABI stability guarantees. It was just that encoraging using the new napi_register_module_vX symbols based approach to register the module.

This method will not be removed in the original intention.

[nodejs-github-bot]

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

[mertcanaltin]

LGTM

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in ca69d0a