Review requested:

• @nodejs/gyp
• @nodejs/node-api

From the PR description this seems super valuable and a great built-in alternative to the weak-node-api library we've been working on to add Node-API support to React Native. As such, I'd be happy for us to adopt this approach over the stuff we have now 👍

The limitation around the module being bound to a single runtime, might not be an issue as a multi-runtime host can inject functions that deal with that internally.

I'm left wondering how (if at all) add-ons which are statically linked into the process are affected by this proposal? I guess not at all, as they can still register themselves and call into the global Node-API functions resolved at link time.

nice! node-api symbol visibility has been a pain for us in deno so we'd love to adopt this approach as well.

There might be a potential a potential coordination problem, where the addon tries to initialize itself before the vtable gets injected (set) by the host. I don't see that as an issue when the addon relies on "symbol based" module registration, since the host can ensure to call the initialize function after setting the vtable, but how about an addon trying to call napi_module_register when loaded?

but how about an addon trying to call napi_module_register when loaded?

these should probably be exclusive modes, so napi_module_register wouldn't be called, or could only be called from within the host call to the inject function.

There might be a potential a potential coordination problem, where the addon tries to initialize itself before the vtable gets injected (set) by the host. I don't see that as an issue when the addon relies on "symbol based" module registration, since the host can ensure to call the initialize function after setting the vtable, but how about an addon trying to call napi_module_register when loaded?

The napi_module_register usage is deprecated. No new code is supposed to use it anymore.
We used to have a deprecated attribute, but we found that some developers use it to register modules explictly. E.g. when the modules are part of the host executable.
I do not have a good answer to that besides that the exisitng public API is still there and user can use as before.

I am going to add a conditional flag and restore the deleted test as @legendecas suggested. This test is using the napi_module_register method and it can be used as a show case how to use the Node-API directly when needed.

I'm left wondering how (if at all) add-ons which are statically linked into the process are affected by this proposal? I guess not at all, as they can still register themselves and call into the global Node-API functions resolved at link time.

Right, I would expect it too, but we should verify and test this scenario.

The issues that I am facing on Mac and Linux are due to the use of real "C" compilers where the inline keyword has a different semantic than in "C++". Changing it to static inline generates its own set of issues. Thus, I am still working on it.

Nice!👍 In Lynx/PrimJS, we are currently using a similar vtable approach to address the needs of multi-runtime injection. However, our previous API was not fully aligned with the Node-API standard, which is a problem I have been working to fix recently.
We’re thrilled to see this solution will potentially be natively integrated into Node.js, and we’re also more than happy to adopt this approach.

Nice!👍 In Lynx/PrimJS, we are currently using a similar vtable approach to address the needs of multi-runtime injection. However, our previous API was not fully aligned with the Node-API standard, which is a problem I have been working to fix recently. We’re thrilled to see this solution will potentially be natively integrated into Node.js, and we’re also more than happy to adopt this approach.

It is great to hear it! Any suggestions to improve code in this PR to fit your scenario are welcome.

Codecov Report

❌ Patch coverage is 98.73418% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.21%. Comparing base (dbec31c) to head (e77aa73).

@@            Coverage Diff             @@
##             main   #60916      +/-   ##
==========================================
- Coverage   90.28%   90.21%   -0.07%     
==========================================
  Files         730      730              
  Lines      234802   234963     +161     
  Branches    43953    44077     +124     
==========================================
- Hits       211991   211980      -11     
+ Misses      14530    14497      -33     
- Partials     8281     8486     +205     

... and 35 files with indirect coverage changes

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

👍
Just as a comment, I was working on something very similar, but I must admit that imho this is the most flexible approach, which opens new possibilities, like enabling validation layers and middlewares (the runtime can "replace" or "patch" the original Node-API function pointer with functions that perform extra validation or logging, etc. completely transparently to the "consumer") -- a little bit like optional Validation Layers in Vulkan API. Lastly, V-table approach also decreases the cost of dynamic symbol resolution, etc.

From ABI stability, adding new function pointers at the end of the struct/V-table is ABI-stable. Lastly, there are some runtimes that support multiple JS engines, and they would need to dispatch to the correct V-table (based on the node_env) -- I believe both PrimJS and OpenHarmony's Native Engine are doing this. This is also one reason why JS engines should not provide Node-API symbols, otherwise they will trigger symbol conflicts.

What might be good to consider is adding a function that returns a V-table for a given version. This way, if a Node Addon is using say N-API version 8, and the runtime supports, say, N-API version 10, such node_api_get_vtable(NAPI_VERSION_8, flags) might return a properly initialized V-Table with newer functions replaced with an assertion or something. Just a random idea inspired by an old blog post...

edit: If we can bump NAPI_MODULE_VERSION then maybe its the best time to introduce napi_register_module_v2() which takes a 3rd argument: a versioned struct/v-table with function pointers to query the host/runtime (e.g. ask for V-Tables, require other addons, etc.). WDYT?

edit: If we can bump NAPI_MODULE_VERSION then maybe its the best time to introduce napi_register_module_v2() which takes a 3rd argument: a versioned struct/v-table with function pointers to query the host/runtime (e.g. ask for V-Tables, require other addons, etc.). WDYT?

@mani3xis , thank you for the feedback and great ideas! In the last few weeks I was working on improving the code and was able to achieve good results that should now enable multiple runtimes in the same executable since the v-table is now exposed from the napi_env as you suggested.
As for the versioning, I played with the idea to add the napi_register_module_v2, but seemed to be unnecessary.
When we currently load a Node-API module we first look for an optional function with the name node_api_module_get_api_version_v1. This function must return the Node-API version supported by the module. If the function is not present, then we assume the Node-API version 8. The version 8 was there when we added this mechanism. Then, the internal napi_env created with the module version inside. Based on that version Node.js adjusts the behavior of the Node-API.
So, when the napi_register_module_v1 is called with the napi_env, that napi_env is already adjusted for the requested version.
Since the v-table in the latest iteration of this PR is a field of napi_env__, it can have the adjusted v-table if needed.

@vmoroz Hi, Could I experimentally try out the solution proposed in this PR in my project first? I’ve been struggling with symbol conflicts between multiple NAPI implementations lately (callstackincubator/react-native-node-api#341), and this solution seems like it could fix my problem—I can’t wait to give it a shot.

@vmoroz Hi, Could I experimentally try out the solution proposed in this PR in my project first? I’ve been struggling with symbol conflicts between multiple NAPI implementations lately (callstackincubator/react-native-node-api#341), and this solution seems like it could fix my problem—I can’t wait to give it a shot.

Hi @RobinWuu , yes, feel free to try it. I am curious to know if it works for you. At this point I consider the implementation to be quite complete. Though it is still a subject of a code review, addressing feedback, etc. I cannot predict when and if at all it will be merged to Node.js project. I.e. there is some risk in using it at the early stage.

Beyond my expertise to review, but based on the frictions it clears regarding integrating Node-API into React Native and Lynx, I'm really looking forward to this. Hope it can be merged!

I believe this now has use-cases for:

• React Native (used by Meta, Microsoft, Amazon and more)
• Lynx/PrimJS (used by ByteDance, as commented by @RobinWuu above)
• NativeScript
• napi-rs (in fact, they called it one of the most frustrating issues they faced)

@toyobayashi would this be useful for your many-target Node-API implementation, emnapi, as well?

Hope this can be re-reviewed sometime soon as it would have widespread impact cross-ecosystem.

@shirakaba In emnapi, the legacy approach to function symbol resolution is to directly import most Node-API functions from the JavaScript world using wasm imports (like WebAssembly.instantiate(module, { env: { ...nodeApiSymbols } })). However, the remaining runtime code for the Node-API implemented in C (such as libuv_threadpool, async_work, tsfn, and async_cleanup_hook) must be statically linked into the final wasm file. This means that if emnapi updates its version to modify these functionalities, users must recompile the code.

I believe the ideal approach would be:

1. All implementations are 100% JavaScript, with no C used. However, this seems unlikely. Leaving aside libuv emulation and static variable initialization, for example, this PR changes the declaration of napi_env to no longer be a transparent pointer, emnapi will no longer be able to map napi_env to JS objects using numeric IDs in JavaScript. In other words, napi_env must have a concrete shape in the linear memory of the wasm module.

or

2. Allow C code to exist, but pre-compile it as a side module (similar to a shared library), sharing linear memory and the wasm table with the user-compiled main module after being loaded at runtime. Using the vtable implementation in this PR, Node-API functions will be placed in the wasm table. This also avoids the wasm module statically importing a large number of symbols.

The vtable provides a possibility for the second point. However, there are several potential problems:

1. When async_work or multithreading is involved, different workers cannot share the wasm table. It's possible that the vtable only works correctly on the main thread. Further exploration is needed here.

2. wasm has very poor support for dynamic libraries, and side modules also impose some restrictions on how users compile the main module. Implementing a dynamic library magic loader in JavaScript is also a challenge for me, and it might not be worth the effort.

3. Emscripten's JavaScript library DCE will fail because the runtime always needs to link all JavaScript implementations of Node-API functions.

Overall, the vtable pattern offers some inspiration for emnapi in solving runtime ABI compatibility issues, and I will actively follow the changes here to achieve the same behavior as much as possible. I believe vtables are of great significance to the Node-API ecosystem. Excellent work!

BTW: I'm still really excited about the prospects of this landing some day 😅 It would make building Node-API addons for React Native much more flexible and remove a silly workaround needed to satisfy the Android linker at runtime.