Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Add Node-API to Hermes #1377

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vmoroz
Copy link
Contributor

@vmoroz vmoroz commented Apr 18, 2024

Summary

This is an initial implementation of Node-API for Hermes.
The code is taken from the hermes-windows repo.

Node-API is an ABI-safe that is originally implemented for Node.js addons, and then adopted by all major JS runtimes.

This is a draft PR. Before removing the draft status I would like to ask a few questons:

  • What must be the library that exposes the Node-API? In hermes-windows we expose it for the hermes.dll. It is the libhermes for Windows. In this repo libhermes is not a shared library.
  • Please advise on the source file names and their locations.
  • What must be file headers?
  • I am going to add the test files after figuring out what must be the library that exposes Node-API.

Test Plan

Unit tests for Node-API are to be added.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 18, 2024
@tmikov
Copy link
Contributor

tmikov commented Apr 18, 2024

Thanks for doing this @vmoroz!!!

@facebook-github-bot
Copy link
Contributor

@tmikov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@shirakaba
Copy link

Thanks so much, @vmoroz! Really excited to play with this.

@NathanWalker
Copy link

Huge effort @vmoroz thank you ❤️

@neildhar
Copy link
Contributor

Hey @vmoroz, thanks for putting this together!

Two high level requests:

  1. Could you move all the new code into the node-api directory? It can produce a static library that we link into libhermes, but I think keeping the implementation of this API separate makes more sense (similar to how the hermes_abi implementation is separate).
  2. As a first step, could we just include the implementations of the actual NAPI APIs? That is, we should exclude the extensions for providing things like CDP integration and access to Hermes specific functionality. We want to support these things eventually, but we haven't decided on what the ABI boundary should look like.

@vmoroz
Copy link
Contributor Author

vmoroz commented Jun 14, 2024

Hey @vmoroz, thanks for putting this together!

Two high level requests:

  1. Could you move all the new code into the node-api directory? It can produce a static library that we link into libhermes, but I think keeping the implementation of this API separate makes more sense (similar to how the hermes_abi implementation is separate).
  2. As a first step, could we just include the implementations of the actual NAPI APIs? That is, we should exclude the extensions for providing things like CDP integration and access to Hermes specific functionality. We want to support these things eventually, but we haven't decided on what the ABI boundary should look like.

@neildhar , thank you for the feedback and suggestions!
Extracting the Node API related code into the node-api folder similar to the hermes_abi code makes perfect sense. I will do that.

@wcandillon
Copy link

wcandillon commented Jul 11, 2024

Is this the main place to follow work on this topic? Is there still some appetite to support NAPI in React Native?
We are eyeing at some node modules that we would love to use out of the box in React Native.

@shirakaba
Copy link

shirakaba commented Jul 11, 2024

I'm still very interested in it and have even submitted conference talk proposals on the topic so would love it to become a reality!

I have some native Node addons that I've written for Electron, and it's a shame that I can use them in React Native Windows (as it supports Node-API) yet can't use them in any other flavour of React Native currently.

Let me know if there's any way I could help!

@vmoroz
Copy link
Contributor Author

vmoroz commented Jul 11, 2024

Is this the main place to follow work on this topic? Is there still some appetite to support NAPI in React Native? We are eyeing at some node modules that we would love to use out of the box in React Native.

One of the Discord RN channels or a discussion topic in one of the Meta's or Microsoft's related repo could be a better place to discuss all the details, but we can start it here.

The goal of supporting Node-API for Hermes, V8, and RN was to provide the industry standard ABI-safe API. It should enable versioning support for modules, support for multiple programming languages, and reuse of native code written for Node.js.

I wonder what is your scenario in terms of tergeting platforms (Windows, Mac, Android, etc.) and if you own the code that uses Node-API? It may be not possible to reuse binary Node.js modules, but reusing the majority of the code and recompiling it for the RN is a more achievable scenario.

@wcandillon
Copy link

wcandillon commented Jul 11, 2024 via email

@vmoroz
Copy link
Contributor Author

vmoroz commented Jul 11, 2024

We are implementing WebGPU for React Native ( https://x.com/appjsconf/status/1806332497783058900) and right now we are virtually migrating a webgpu node module to JSI manually. So of course the thought or being able to use that node module directly is appealing to us.

This is awesome! In the video clip you say that you target first of all the mobile platforms: iOS and Android.
It means that you cannot use the precompiled binaries and targeting the Node-API is mostly the matter of the convenience of reusing the existing code. IMHO, it is an achievable goal and @shirakaba's team is already using the Hermes Node-API for their project. The only problem is that to use it we must modify the Hermes code to integrate with the jsi::Runtime.
This PR targets to eliminate this issue and to enable Node-API out of the main Hermes repo.

The current version of the Node-API for Hermes is in the hermes-windows repo: https://github.com/microsoft/hermes-windows/ . There we are starting to factor out the Node-API code into its own folder.

Yesterday we had a long informative face-to-face meeting with Hermes team.
The Hermes team seems to be still in the favor of supporting Node-API. There was an interesting discussion about the interoperability between Node-API and jsi::Runtine/AbiRuntime. There are a few issues to address there. Hopefully we can do it soon.

@tmikov
Copy link
Contributor

tmikov commented Jul 12, 2024

@wcandillon when you say that you are implementing WebGPU, do you literally mean the WebGPU spec, 1 to 1, with shader language, etc?

@wcandillon
Copy link

wcandillon commented Jul 12, 2024

We use Google Dawn for the native WebGPU implementation (Dawn is also a supported Skia backend), we code generate most of the bindings from its TS definition (which is itself generated from the W3C spec, we also match TS methods to C++ methods, Dawn provides a json model for that), and for the manual part of the work we just look at the node module and migrate it. This is why being able to use that node binding directly (which is already conformant to the official WebGPU testsuite) feels appealing.

@tmikov
Copy link
Contributor

tmikov commented Jul 12, 2024

May I emphatically object to using Discord channels, as they inherently make discussions exclusionary, unfriendly to different timezones, transient, difficult to search, and impossible to reference.

@SamuelScheit
Copy link

For anyone who wants to try out this PR with react-native use version 0.75.0-rc.5 (or newer).

@SamuelScheit
Copy link

I was able to link and run a react-native app with this hermes NodeAPI PR.

However after calling hermes_create_napi_env it fails with EXC_BAD_ACCESS and the following stacktrace:

hermes::CompactArray::Fixed<unsigned char>::get(char*, unsigned int) [inlined]
hermes::CompactArray::get(unsigned int) const [inlined]
hermes::CompactTable::isEmpty(unsigned int) const [inlined]
unsigned int hermes::vm::detail::IdentifierHashTable::lookupString<char>(llvh::ArrayRef<char>, unsigned int, bool) consthermes_node_api/lib/VM/detail/IdentifierHashTable.cpp:41
hermes::vm::SymbolID hermes::vm::IdentifierTable::registerLazyIdentifierImpl<char>(llvh::ArrayRef<char>, unsigned int)hermes_node_api/lib/VM/IdentifierTable.cpp:393
hermes::napi::NapiEnvironment::NapiEnvironment(hermes::vm::Runtime&, bool, std::__1::shared_ptr<facebook::jsi::PreparedScriptStore>, hermes::vm::RuntimeConfig const&)Users/user/Developer/respond/hermes/hermes_node_api/API/hermes/hermes_napi.cpp:3330
hermes::napi::NapiEnvironment::NapiEnvironment(hermes::vm::Runtime&, bool, std::__1::shared_ptr<facebook::jsi::PreparedScriptStore>, hermes::vm::RuntimeConfig const&) [inlined]
::hermes_create_napi_env(void *, napi_env *)

raw is a NULL pointer and therefore results in EXC_BAD_ACCESS

static uint32_t get(char *raw, uint32_t idx) {
return reinterpret_cast<T *>(raw)[idx];
}

uint32_t get(uint32_t idx) const {
assert(idx < size_);
switch (scale_) {
case UINT8:
return Fixed<uint8_t>::get(raw_, idx);

bool isEmpty(uint32_t idx) const {
return CompactArray::get(idx) == EMPTY;
}

if (table_.isEmpty(idx)) {

auto idx = hashTable_.lookupString(str, hash);

https://github.com/vmoroz/hermes-windows/blob/5d4656887af7397700006e1beec28dc9abd3333f/API/hermes/hermes_napi.cpp#L3327-L3331

so what I'm currently trying to figure out is: why is raw a null pointer and what is the source of this issue

Does anybody else tried to use the NAPI in an app and if yes were you successful or did you encounter errors?

@SamuelScheit
Copy link

After building hermes in debug mode I get a different error, however without any stacktrace:

libc++abi: terminating due to uncaught exception of type std::length_error: vector

and after retrying a few times a different error:

libc++abi: terminating due to uncaught exception of type std::bad_alloc

Does anyone know how I can get the stacktrace of these errors to find the cause??

@kraenhansen
Copy link
Contributor

kraenhansen commented Nov 18, 2024

With @shirakaba making noise at React Summit US on Friday, I rediscovered my excitement around the prospective future of having access to the Node-API symbols for my current and future React Native native modules.

I help maintain the Realm database, supporting RN and Node.js. Landing this PR would mean we could 1) maintain only one binding between our native code and a runtime API across the platforms we support and 2) unlock publishing this binding code as a prebuild. Ultimately leading to much improved developer experience for us and our users!

This PR is crucial to making this future a reality and I would love to help move this forward:

  • In particular the description mentions having to add tests, but I can't tell if you already have tests implemented in the MS codebase that just needs to be copied or if this is something I can help write up?
  • I might be able to run Realm's unit tests with the Node-API binding in a React Native app running with Hermes from this PRs branch.
  • Making some noise to help making it obvious how important this work is 💙

What would you think would be most impactful?

@kraenhansen
Copy link
Contributor

kraenhansen commented Nov 21, 2024

A quick update on my side: I believe I'm making good progress on getting the Realm's unit tests running with the Node-API binding in a React Native app, with Hermes from this PRs branch. I had to apply a few workarounds, but I believe most were to address limitations in the running React Native app.

Our native module is using the C++ wrapper, more specifically the Napi::Addon abstraction to define itself.
Bringing in those headers results in (so far) two undefined symbols when linking:

I don't see these listed as TODOs in hermes_napi.cpp

// TODO: Allow DebugBreak in unexpected cases - add functions to indicate
// expected errors
// TODO: Create NapiEnvironment with JSI Runtime
// TODO: Fix Inspector CMake definitions
// TODO: Cannot use functions as a base class
// TODO: NativeFunction vs NativeConstructor
// TODO: Different error messages
// TODO: Arrays with 2^32-1 elements (sparse arrays?)
// TODO: How to provide detailed error messages without breaking tests?
// TODO: Why console.log compiles in V8_JSI?

@kraenhansen
Copy link
Contributor

kraenhansen commented Nov 23, 2024

I've come to realize the PR provides implementations for the "js_native_api.h" part of Node-API only, leaving these 30 functions undefined:

  • napi_async_destroy
  • napi_async_init
  • napi_cancel_async_work
  • napi_create_async_work
  • napi_create_buffer
  • napi_create_buffer_copy
  • napi_create_external_buffer
  • napi_delete_async_work
  • napi_fatal_error
  • napi_get_buffer_info
  • napi_get_node_version
  • napi_is_buffer
  • napi_make_callback
  • napi_module_register
  • napi_queue_async_work
  • napi_get_uv_event_loop
  • napi_add_env_cleanup_hook
  • napi_close_callback_scope
  • napi_fatal_exception
  • napi_open_callback_scope
  • napi_remove_env_cleanup_hook
  • napi_acquire_threadsafe_function
  • napi_call_threadsafe_function
  • napi_create_threadsafe_function
  • napi_get_threadsafe_function_context
  • napi_ref_threadsafe_function
  • napi_release_threadsafe_function
  • napi_unref_threadsafe_function
  • napi_add_async_cleanup_hook
  • napi_remove_async_cleanup_hook

These are all declared in the top-level node_api.h.

My guess is that this is because these functions aren't needed in the use-case of React Native for Windows, is that correct?
Do you have implementations of these elsewhere that could be included in the PR or will they need new implementations to make the Node-API support complete?

@tmikov
Copy link
Contributor

tmikov commented Nov 26, 2024

@kraenhansen I know very little about Node-API, but my guess is that some (all?) of the missing APIs are NodeJS-specific.

For example, napi_get_uv_event_loop doesn't even make sense outside of a libuv-based integration. napi_create_buffer returns a node::Buffer object. Ditto for napi_get_node_version. None of these belong in Hermes.

@kraenhansen
Copy link
Contributor

kraenhansen commented Nov 26, 2024

@tmikov You're right that the implementation of some of these are trivial, as they should likely simply error.

None the less, they're all a part of the specification and need some implementation. Without it, an addon relying on these features won't be able to perform runtime-checks for the availability of those features and will instead experience build errors.

@tmikov
Copy link
Contributor

tmikov commented Nov 26, 2024

@kraenhansen I am sorry for being unclear. I did not say that these APIs are simple. I said that some of these APIs are specific to Node.js and so they cannot be part of Hermes.

  • napi_get_uv_event_loop deals with libuv. It has nothing to do with Hermes.
  • napi_create_buffer deals with Node::buffer. Again, not relevant to Hermes.
  • napi_get_node_version returns version information about Node.js. Hermes is not Node.js.

The purpose of this PR is not to create an emulation of NodeJS. Instead the goal is to implement the engine-specific parts of the API.

@Brooooooklyn
Copy link

For reference, napi_get_uv_event_loop returns nullptr in emnapi, which is very intuitive because it doesn't have a libuv runtime.

For napi_get_node_version, Deno returns 20.11.1, and this version can represent the range of Node-API supported by Deno. Similarly, Hermes can also take similar strategy, and for users of Node-API, this API is usually to determine the support for Node-API.

The napi_create_buffer function can be a bit complex, so I suggest adding a new type of error status to Node.js, similar to napi_no_external_buffers_allowed. Let’s call it node_api_no_buffer. This would help library authors because if napi_create_*_buffer returns node_api_no_buffer, they will know that the runtime environment doesn’t support Buffer. This way, they can use napi_create_*_typedarray for compatibility purposes.

@tmikov
Copy link
Contributor

tmikov commented Nov 27, 2024

@Brooooooklyn Hermes is only the JS engine, functionally equivalent to v8 or JavaScriptCore, for example. It is not a complete runtime environment like NodeJS, Deno, Bun, etc. The JS engine is just one part of a runtime environment.

Hermes is also not trying to be a complete implementation of Node-API like emnapi.

People can build runtimes by using Hermes as one component of the runtime, and can implement the full Node API on top of it, if they want to. I admit, that would be very cool, actually. But in any case, Hermes itself has a very narrow function - to execute JS.

The PR aims to expose the JS engine-specific parts of Node-API. It is not meant to emulate NodeJS for the purpose of running existing native extensions.

Again, such a thing can be built on top of Hermes by third parties.

@kraenhansen
Copy link
Contributor

@kraenhansen I am sorry for being unclear. I did not say that these APIs are simple.

Re-reading my answer, I seem to be the one being unclear 🙈 I was trying to hint that for Hermes most of these could be implemented as no-ops which would simply return an error status. But thinking more about this however: That would take away the opportunity for some other component to provide an actual implementation for those symbols 🤔

I think what confused me the most was, that the description of the PR stated that it was implementing Node-API for Hermes, while in-fact there were a large part of the spec that wasn't actually being implemented - this seemed like an oversight to me.

That being said, while I understand hermes might not be the place to implement this, I'd expect most consumers of this to be trying to rebuild existing Node native modules for Hermes / React Native and I'd expect those to use the C++ node-addon-api headers, which would fail to link.

Someone in the community could probably implement a native module for React Native, to provide (at least a dummy implementation of) the rest of the symbols.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants