Skip to content

Conversation

@kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Sep 12, 2025

We're entering the "how did this ever work" territory.

Merging this PR will:

  • Remove the explicit linking of the host with the weak-node-api library to prevent the host from injecting the weak-node-api symbols into itself.
  • Rename the ferric-example package in the test app - I have no clue why this passed on main 😬 since d367b6e merged (perhaps the package-lock.json acted as a cache somehow).

This should fix the crash I've been experiencing on #225.

@kraenhansen kraenhansen self-assigned this Sep 12, 2025
@kraenhansen kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) labels Sep 12, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Removes explicit weak-node-api linking on Android to prevent symbol injection crashes and switches to dynamic loading approach.

  • Removes static linking of weak-node-api library from CMakeLists.txt
  • Changes code to use namespaced function calls instead of global Node API functions
  • Configures build system to include weak-node-api for dynamic loading

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
packages/host/cpp/RuntimeNodeApi.cpp Updates function call to use namespaced version
packages/host/android/build.gradle Adds weak-node-api to jniLibs for dynamic loading
packages/host/android/CMakeLists.txt Removes static linking, converts to interface library
.changeset/upset-papayas-pump.md Documents the fix in changelog

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

main {
manifest.srcFile "src/main/AndroidManifestNew.xml"
// Include the weak-node-api to enable a dynamic load
jniLibs.srcDirs = ["../weak-node-api/weak-node-api.android.node"]
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jniLibs.srcDirs assignment overwrites any existing source directories. Use jniLibs.srcDirs += [\"../weak-node-api/weak-node-api.android.node\"] to append to existing directories instead of replacing them.

Suggested change
jniLibs.srcDirs = ["../weak-node-api/weak-node-api.android.node"]
jniLibs.srcDirs += ["../weak-node-api/weak-node-api.android.node"]

Copilot uses AI. Check for mistakes.
@kraenhansen kraenhansen force-pushed the kh/android/no-explicit-weak-node-api-linking branch from a1a5d5a to e30e625 Compare September 12, 2025 21:45
@kraenhansen
Copy link
Collaborator Author

For some reason building fails on iOS even touched just a single line of common C++, which I strongly believe is unrelated.
Also, I'm able to build for iOS locally, so instead of getting this to a passing state, I'll merge #162 which is already green and push the changesets from this PR onto main directly after. Please review this in retrospect and we can follow up with PRs if there's anything we need to change 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants