Injecting Node-API functions into `weak-node-api` #49

kraenhansen · 2025-05-10T12:16:00Z

Merging this PR will:

Change weak-node-api to have the Node-API functions injected from the host, instead of resolving them itself.
Load weak-node-api into the process without polluting the global symbol registry, when the host injects the Node-API functions from libhermes and the host (which is future work).
Make the host consume weak-node-api as a pre-built, like the Node-API modules are already doing.
Strip the "lib" prefix from prebuild directory names, if a project generates a lib-prefixed binary.
Add a cross-platform log_debug function (which we'll likely want to mute in the future).
Fix outstanding linking issues, making Node-API modules built with our CMake wrapper or Ferric, load and register on iOS and Android.

Here's at generated files: https://gist.github.com/kraenhansen/049fac2b03daacab1ebc41ca425920ca

changeset-bot · 2025-05-10T13:03:03Z

⚠️ No Changeset found

Latest commit: 1862e3b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

kraenhansen · 2025-05-10T16:36:12Z

packages/react-native-node-api-modules/scripts/generate-weak-node-api.ts

+    "#include <node_api.h>", // Node-API
+    "#include <stdio.h>", // fprintf()
+    "#include <stdlib.h>", // abort()
+    "namespace node_api::internal {",


I wonder if this is too intrusive? Should keep this under callstack:: instead or is this okay? I mean, ideally this would be usable outside of our setup.

Removed this as it served little purpose now that the inject function is top-level.

Now that I'm no longer using a namespace, nor any C++ features, I'm thinking about making this an actual .c file to make sure I'm not using any C++ features. IDYT @mani3xis?

mani3xis

I found a major issue (relying on mangled name) and a few minors and nits. Overall looks good!

mani3xis · 2025-05-15T10:22:57Z

packages/ferric/src/cargo.ts

+  "aarch64-linux-android": "arm64-v8a",
+  "armv7-linux-androideabi": "armeabi-v7a",
+  "i686-linux-android": "x86",
+  "x86_64-linux-android": "x86_64",


nit: What about making a type for the values to avoid typos? I'm sure that you listed all values already, so "arm64-v8a" | "armeabi-v7a" | "x86" | "x86_64" should do the trick, wdyt?

nit 2: Maybe we should pick a more general/unified way of describing targets / cpu archs? I'm afraid that adding new platforms will case an explosion of enums.

And use that type instead of string in the Record above? I don't see that I will be referencing that type elsewhere 🤔 Where would you use that type? Perhaps a suggestive edit would help me understand your intent.

mani3xis · 2025-05-15T10:24:41Z

packages/ferric/src/cargo.ts

+    const weakNodeApiPath = getWeakNodeApiAndroidLibraryPath(target);

    return {
+      RUSTFLAGS: `-L ${weakNodeApiPath} -l weak-node-api`,


Nit: Hopefully there are no spaces in the path (or at least the string gets quoted in such case)

You're right - great catch. This should be quoted 👍

Hmm - it failed when I quoted it (either with " or ') 🤔 It seems CARGO_ENCODED_RUSTFLAGS is what I'd have to use 😆

mani3xis · 2025-05-15T10:27:29Z

packages/react-native-node-api-cmake/src/cli.ts

-              }
-            });
-            assert.equal(result.length, 1, "Expected exactly library file");
+            const result = readdirSync(tripletOutputPath, {


Minor: Perhaps we can use asynchronous functions instead and use await Promise.all(). This should help performance a bit.

I turned all the sync calls (except existsSync) into async I/O 👍

mani3xis · 2025-05-15T10:28:56Z

packages/react-native-node-api-modules/.gitignore

 /weak-node-api/*.android.node
-/weak-node-api/weak-node-api.cpp
+/weak-node-api/weak_node_api.cpp
+/weak-node-api/weak_node_api.hpp


Nit: /weak-node-api/weak_node_api.[ch]pp if you like one-liners

mani3xis · 2025-05-15T10:37:41Z

packages/react-native-node-api-modules/scripts/generate-weak-node-api-injector.ts

+
+    namespace callstack::nodeapihost {
+    
+    using node_api::internal::inject_host_t;


nit: The _t suffix is reserved by POSIX but this is not a big deal breaker. I would recommend to follow the convention and use CamelCase for types (as used in React and rest of our code), like NodeApiHost type below.

Okay, thanks 👍 How about InjectHostFunction?

mani3xis · 2025-05-15T10:55:29Z

packages/react-native-node-api-modules/scripts/generate-weak-node-api.ts

+    // Generate function calling into the host
+    ...functions.flatMap(({ returnType, name, argumentTypes }) => {
+      return [
+        `extern "C" ${returnType} ${name}(${argumentTypes


Missing calling convention specifier and there's one function that does not return (napi_fatal_error IIRC) which also had noreturn attribute.

Moreover, I would highly advise to keep the NAPI_VERSION-specific #if...#endif macros in the generated source. We would be able to control which functions are "visible" and therefore generate errors when somebody tries to use function that we do not support.

keep the NAPI_VERSION-specific #if...#endif macros in the generated source

We talked about this in person, right?

As I expect this code to be compiled into a single prebuilt and linked to from multiple Node-API modules (each with their own NAPI_VERSIONs to restrict their available API), I don't see a scenario where we'd want to provide a prebuilt with symbols less than NAPI_VERSION 10. To me the constant is entirely a "consumer side" concept. (And I might be wrong about that?)

Missing calling convention specifier

What more than extern "C" do we need in your opinion?

I'll make sure to add the noreturn 👍

mani3xis · 2025-05-15T11:13:03Z

packages/react-native-node-api-modules/scripts/generate-weak-node-api.ts

+          .join(", ")}) {`,
+        `if (g_host.${name} == nullptr) {`,
+        `  fprintf(stderr, "Node-API function '${name}' called before it was injected!\\n");`,
+        "  abort();",


Maybe we should introduce a "backdoor" which would allow users to configure how to behave in such a case? For example, in my first draft which I've generated earlier this week I'm using the ASSERT() macro for such checks (yet I'm looking for a better name). This can be a fallback to assert() from assert.h or whatever the user would like to have. Might be a great addition for testing/mocking.

would allow users to configure how to behave in such a case

Again, I expect us to ship the prebuild for this and I therefore expect that to be outside of the control of users.

That being said, I'd be happy to extend this, once I see a clear need 👍
I mean, we would want something relying on that behavior if we add it in.

mani3xis · 2025-05-15T11:20:06Z

packages/react-native-node-api-modules/scripts/node-api-functions.ts

+  fallbackReturnStatement: string;
+};
+
+export function getNodeApiFunctions(version: NodeApiVersion = "v8") {


Nit: I understand where this is coming from (the default version when not specified), although wouldn't it be better to start with v1 which we are not supporting yet?

I am contemplating moving this to a top-level constant instead. As mentioned above, I don't understand the need for weak-node-api to be built with less than the highest number available - I might actually want to bump it to v10 🙈

Would you rather that prebuilds to fail compiling instead of us aborting at runtime? (Less and less as we increase our support).

mani3xis · 2025-05-15T11:23:33Z

packages/react-native-node-api-modules/weak-node-api/CMakeLists.txt

+    ${CMAKE_CURRENT_SOURCE_DIR}/include
+)
 target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17)
 target_compile_definitions(${PROJECT_NAME} PRIVATE NAPI_VERSION=8)


Nit: As mentioned earlier, shouldn't we start with NAPI_VERSION=1? Moreover hardcoding the version here might not be a good idea, as it would be hard to generate prebuilds for various versions (like with node-pre-gyp and friends).

Same as above ☺️

mani3xis · 2025-05-15T11:30:06Z

packages/react-native-node-api-modules/scripts/generate-weak-node-api.ts

+        `extern "C" ${returnType} ${name}(${argumentTypes
+          .map((type, index) => `${type} arg${index}`)
+          .join(", ")}) {`,
+        `if (g_host.${name} == nullptr) {`,


Comment (just to leave the trace from our yesterday's call): This will work for (the deprecated) napi_register_module() only when weak-node-api is built as shared-library and only if we guarantee that host injected the functions before the add-on is loaded! In earlier modules, there was a macro which generated a function that would call this napi_register_module(), and it was decorated with __attribute__((constructor)), meaning that a call to napi_register_module() happened BEFORE dlopen() returned, leaving no time to inject functions (when statically linked).

Didn't we talk about this and realized this would only be an issue on a platform where all dynamic libraries are preloaded on app startup (iOS)? I mean, we'd expect to be in control of when the dlopen of the Node-API module is happening and we can make sure the functions are injected beforehand.

…nd move it out of namespace

kraenhansen self-assigned this May 10, 2025

kraenhansen marked this pull request as ready for review May 10, 2025 13:03

kraenhansen force-pushed the kh/weak-node-api-injection branch from a2539b0 to 63d865c Compare May 10, 2025 13:14

kraenhansen force-pushed the kh/ferric branch from e854935 to 17f6af0 Compare May 10, 2025 13:14

kraenhansen linked an issue May 10, 2025 that may be closed by this pull request

Refactor weak-node-api to have Node-API functions injected instead of loading the itself #44

Closed

kraenhansen commented May 10, 2025

View reviewed changes

kraenhansen force-pushed the kh/weak-node-api-injection branch from 63d865c to 9e0f637 Compare May 10, 2025 20:44

kraenhansen force-pushed the kh/ferric branch from 17f6af0 to bf48ac8 Compare May 10, 2025 20:44

kraenhansen force-pushed the kh/weak-node-api-injection branch from 9e0f637 to 9f18d8c Compare May 11, 2025 10:43

kraenhansen force-pushed the kh/ferric branch from bf48ac8 to 34c26c2 Compare May 11, 2025 10:43

kraenhansen force-pushed the kh/weak-node-api-injection branch from 9f18d8c to 86cff81 Compare May 11, 2025 20:36

kraenhansen force-pushed the kh/ferric branch from 34c26c2 to 5978142 Compare May 11, 2025 20:36

kraenhansen force-pushed the kh/weak-node-api-injection branch 2 times, most recently from a438cb8 to 55e73d0 Compare May 11, 2025 22:31

Base automatically changed from kh/ferric to main May 11, 2025 22:48

kraenhansen force-pushed the kh/weak-node-api-injection branch from 55e73d0 to 2a04f9e Compare May 11, 2025 22:49

kraenhansen requested a review from mani3xis May 11, 2025 22:49

kraenhansen force-pushed the kh/weak-node-api-injection branch 3 times, most recently from 6b024e9 to d04ce9e Compare May 12, 2025 00:02

kraenhansen linked an issue May 14, 2025 that may be closed by this pull request

Implement stubs for the missing runtime Node-API symbols #19

Closed

kraenhansen force-pushed the kh/weak-node-api-injection branch from 629a063 to 74b48e1 Compare May 15, 2025 05:53

mani3xis requested changes May 15, 2025

View reviewed changes

kraenhansen added 6 commits May 17, 2025 23:12

Update weak_node_api to use injection

759a4ae

Improve the search for Android .so files

ab66928

Injecting when loading in iOS

3cbf905

Injecting on Android

353fa4f

Consuming the weak-node-api prebuild

9d3719f

Link Android libs from Ferric with weak-node-api

60b1d9e

kraenhansen added 11 commits May 17, 2025 23:12

Strip lib from xcframework and android library directories

f74b673

Strip "lib" prefix from prebuild directory names

7a2f47e

Add cross-platform logging

6ba521e

Load weak-node-api dynamically when injecting

cbb9510

Remove extraneous doNotStrip

3651d7b

Escape weakNodeApiPath

59934ae

More async fs I/O in cmake CLI

e8e8c5d

Rename inject_host_t to InjectHostFunction

e74a133

Rename inject_host to inject_weak_node_api_host, mark it extern "C" a…

2cb0726

…nd move it out of namespace

Error on warnings when building weak-node-api

29614f9

Propagating __attribute__((noreturn)) from headers

9578c63

kraenhansen force-pushed the kh/weak-node-api-injection branch from 74b48e1 to 9578c63 Compare May 17, 2025 21:13

kraenhansen requested a review from mani3xis May 17, 2025 21:28

This was referenced May 17, 2025

Minor fixes from using package outside of a mono-repo #93

Merged

Emit "Ferric" into generated files #89

Merged

Store Apple prebuilds as .apple.node instead of .xcframework #52

Merged

Generate the injector on CI

1862e3b

kraenhansen merged commit 0a638a9 into main May 19, 2025
3 checks passed

kraenhansen deleted the kh/weak-node-api-injection branch May 19, 2025 14:33

mani3xis approved these changes May 19, 2025

View reviewed changes


		namespace callstack::nodeapihost {

		using node_api::internal::inject_host_t;

Injecting Node-API functions into weak-node-api #49

Injecting Node-API functions into weak-node-api #49

Uh oh!

Conversation

kraenhansen commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

kraenhansen May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mani3xis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Injecting Node-API functions into `weak-node-api` #49

Injecting Node-API functions into `weak-node-api` #49

kraenhansen commented May 10, 2025 •

edited

Loading

changeset-bot bot commented May 10, 2025 •

edited

Loading

kraenhansen May 10, 2025 •

edited

Loading

kraenhansen May 17, 2025 •

edited

Loading

kraenhansen May 17, 2025 •

edited

Loading

kraenhansen May 17, 2025 •

edited

Loading

kraenhansen May 17, 2025 •

edited

Loading

kraenhansen May 17, 2025 •

edited

Loading

kraenhansen May 17, 2025 •

edited

Loading