Do NOT deprecate `napi_module_register` or explain better the current way to do it (embedding API)

[viferga]

What is the problem this feature will solve?

I have been embedding NodeJS since version 8.x and it has been a nightmare. We have achieved recently a pretty solid and consistent version, which takes advantage of napi_module_register for embedding it. In the newest versions (at least 18.x) I realized you are planning to deprecate it:

node/src/node_api.h

Line 91 in 7904bc0

// Deprecated. Replaced by symbol-based registration defined by NAPI_MODULE

Please do not do it, or at least explain how to do it from now. Also, I have seen you plan to deprecate node::Start, don't do it neither. I have spent years in order to fully support NodeJS embedding without bugs and in a robust way and you continuously change the API, making it more difficult and forcing me to refactor the whole project. I explained long time ago the methodology I used to embed and I gave some hints here in order to improve it. Right now the only thing I am asking for is to provide backward compatibility with those APIs so I do not have to re-implement it again and again.

Here's my first post: #23265 (comment)

We are one of the few solutions to fully embed NodeJS in a production ready environment and commercially, in order to show numbers to see the difficulty of this, this is the difference of lines of code that took me to embed NodeJS, against Python:

NodeJS:

 find ../source/loaders/node_loader/ -name '*.*' -type f | xargs wc -l
    57 ../source/loaders/node_loader/source/node_loader.c
   462 ../source/loaders/node_loader/source/node_loader_trampoline.cpp
   862 ../source/loaders/node_loader/source/node_loader_port.cpp
  4642 ../source/loaders/node_loader/source/node_loader_impl.cpp
    98 ../source/loaders/node_loader/include/node_loader/node_loader_bootstrap.h
    42 ../source/loaders/node_loader/include/node_loader/node_loader_trampoline.h
    85 ../source/loaders/node_loader/include/node_loader/node_loader_win32_delay_load.h
    78 ../source/loaders/node_loader/include/node_loader/node_loader_impl.h
    38 ../source/loaders/node_loader/include/node_loader/node_loader_port.h
    40 ../source/loaders/node_loader/include/node_loader/node_loader.h
   245 ../source/loaders/node_loader/CMakeLists.txt
    87 ../source/loaders/node_loader/bootstrap/lib/package-lock.json
    87 ../source/loaders/node_loader/bootstrap/lib/.gitignore
   938 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/dist/espree.cjs
   173 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/espree.js
   244 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/README.md
   348 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/lib/espree.js
   265 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/lib/token-translator.js
   122 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/lib/options.js
     3 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/lib/version.js
    27 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/lib/features.js
    82 ../source/loaders/node_loader/bootstrap/lib/node_modules/espree/package.json
    91 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/dist/bin.js
     1 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/dist/acorn.mjs.d.ts
  5605 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/dist/acorn.js
  5574 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/dist/acorn.mjs
   252 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/dist/acorn.d.ts
   273 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/README.md
    50 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/package.json
   810 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn/CHANGELOG.md
     9 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/dist/visitor-keys.d.ts
    17 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/dist/index.d.ts
   382 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/dist/eslint-visitor-keys.cjs
   107 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/README.md
   312 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/lib/visitor-keys.js
    65 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/lib/index.js
    62 ../source/loaders/node_loader/bootstrap/lib/node_modules/eslint-visitor-keys/package.json
    40 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn-jsx/README.md
    12 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn-jsx/index.d.ts
   488 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn-jsx/index.js
   255 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn-jsx/xhtml.js
    27 ../source/loaders/node_loader/bootstrap/lib/node_modules/acorn-jsx/package.json
    51 ../source/loaders/node_loader/bootstrap/lib/node_modules/.package-lock.json
    15 ../source/loaders/node_loader/bootstrap/lib/package.json
   464 ../source/loaders/node_loader/bootstrap/lib/bootstrap.js
   115 ../source/loaders/node_loader/bootstrap/CMakeLists.txt
 24102 total

Python:

find ../source/loaders/py_loader/ -name '*.*' -type f | xargs wc -l
   117 ../source/loaders/py_loader/source/py_loader_threading.cpp
    57 ../source/loaders/py_loader/source/py_loader.c
   980 ../source/loaders/py_loader/source/py_loader_port.c
   174 ../source/loaders/py_loader/source/py_loader_dict.c
  4235 ../source/loaders/py_loader/source/py_loader_impl.c
    42 ../source/loaders/py_loader/include/py_loader/py_loader_threading.h
    69 ../source/loaders/py_loader/include/py_loader/py_loader_impl.h
    40 ../source/loaders/py_loader/include/py_loader/py_loader.h
    40 ../source/loaders/py_loader/include/py_loader/py_loader_port.h
    40 ../source/loaders/py_loader/include/py_loader/py_loader_dict.h
   242 ../source/loaders/py_loader/CMakeLists.txt
  6036 total

If we want to be fair, removing NodeJS dependencies:

find ../source/loaders/node_loader/ -name '*.*' -type f | xargs wc -l
    57 ../source/loaders/node_loader/source/node_loader.c
   462 ../source/loaders/node_loader/source/node_loader_trampoline.cpp
   862 ../source/loaders/node_loader/source/node_loader_port.cpp
  4642 ../source/loaders/node_loader/source/node_loader_impl.cpp
    98 ../source/loaders/node_loader/include/node_loader/node_loader_bootstrap.h
    42 ../source/loaders/node_loader/include/node_loader/node_loader_trampoline.h
    85 ../source/loaders/node_loader/include/node_loader/node_loader_win32_delay_load.h
    78 ../source/loaders/node_loader/include/node_loader/node_loader_impl.h
    38 ../source/loaders/node_loader/include/node_loader/node_loader_port.h
    40 ../source/loaders/node_loader/include/node_loader/node_loader.h
   245 ../source/loaders/node_loader/CMakeLists.txt
    87 ../source/loaders/node_loader/bootstrap/lib/package-lock.json
    87 ../source/loaders/node_loader/bootstrap/lib/.gitignore
    15 ../source/loaders/node_loader/bootstrap/lib/package.json
   464 ../source/loaders/node_loader/bootstrap/lib/bootstrap.js
   115 ../source/loaders/node_loader/bootstrap/CMakeLists.txt
  7417 total

And support for objects and classes in NodeJS is still incomplete, so it means it even will take more lines of code (around 1k more) to fully implement it. Also, I am not considering other nightmares like packaging libnode properly or finding and compiling it for CMake: https://github.com/metacall/core/blob/develop/cmake/FindNodeJS.cmake

We are using NodeJS right now in a commercial FaaS/BaaS, and this would help a lot in order to keep up to date with latest versions of NodeJS.

What is the feature you are proposing to solve the problem?

Remove the [[deprecated]] flag of napi_module_register and mantain backward compatibility for other APIs like node::Start, please.

Otherwise, give a better solution of how to use it with the current API. I have started to implement it but it's highly incompatible with the old embedding API, here is my attempt:
https://github.com/metacall/core/blob/55a8553a6396f7f0791f8ec3921a2da9ccc05ea6/source/loaders/node_loader/source/node_loader_impl.cpp#L912

What alternatives have you considered?

No response

[juanarbol]

cc / @nodejs/node-api

[mhdawson]

We discussed in the team meeting today, and the consensus was that the intent of the deprecation was to encourage people to use the new approach versus actually removing the old way.

We agreed we'll remove the deprecation flag and then add more examples of using the new approach.

@vmoroz volunteered do the first PR to remove the deprecation flag and then explore what added documentation.

[viferga]

Ok, thank you so much. But you plan to deprecate it in the long term? Same for node::Start? Or are you going to keep it backward compatible? At this point I am not interested anymore in the new embedding API.

[legendecas]

The node::Start is not part of ABI stability guarantee, as same as all the V8 C++ APIs. Given that the [[deprecate]] notation has been removed for napi_module_register, I'm closing this issue. Please open a new issue if you have any requests regarding node::Start. Thanks.