-
Notifications
You must be signed in to change notification settings - Fork 6
Auto link on Android #39
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
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the host package CLI to extract non‑Apple linking logic and add support for auto‑linking Android modules, while also implementing a weak‑node‑api using runtime symbol lookup. Key changes include:
- Removal of the "xcframeworks" command and consolidation of linking logic into a unified "link" command
- Addition of Android module linking support and updates to build scripts and keystore bindings for weak‑node‑api
- Adjustments to podspec and native implementations (C++/Kotlin) for proper library naming and linking
Reviewed Changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-node-api-modules/src/node/duplicates.ts | Added a helper to find duplicate values |
| packages/react-native-node-api-modules/src/node/cli/xcframeworks.ts | Removed in favor of new unified linking commands |
| packages/react-native-node-api-modules/src/node/cli/program.ts | Updated CLI command structure for linking Apple and Android modules |
| packages/react-native-node-api-modules/src/node/cli/options.ts | Added a new option to strip path suffixes |
| packages/react-native-node-api-modules/src/node/cli/link-modules.ts | New file for handling module linking logic |
| packages/react-native-node-api-modules/src/node/cli/hermes.ts | Adjusted import for pretty paths |
| packages/react-native-node-api-modules/src/node/cli/apple.ts | New file for Apple-specific linking via xcframeworks |
| packages/react-native-node-api-modules/src/node/cli/android.ts | New file for Android-specific linking logic |
| packages/react-native-node-api-modules/scripts/build-weak-node-api.ts | Updated weak-node-api build script with dynamic symbol resolution |
| packages/react-native-node-api-modules/react-native-node-api-modules.podspec | Updated vendored frameworks path to support auto-linked frameworks |
| packages/react-native-node-api-modules/cpp/CxxNodeApiHostModule.cpp | Updated Android library path naming logic |
| packages/react-native-node-api-modules/android/src/main/java/com/callstack/node_api_modules/NodeApiModulesPackage.kt | Adjusted native library loading sequence |
| packages/react-native-node-api-cmake/src/weak-node-api.ts | Updated naming for weak-node-api xcframework/library |
| packages/react-native-node-api-cmake/src/android.ts | Modified library name extraction during copy |
| packages/node-addon-examples/scripts/build-examples.mts | Updated CLI invocation parameters for supported triplets |
Files not reviewed (3)
- packages/react-native-node-api-modules/.gitignore: Language not supported
- packages/react-native-node-api-modules/android/CMakeLists.txt: Language not supported
- packages/react-native-node-api-modules/android/build.gradle: Language not supported
| assert(libraryDirents.length === 1, "Expected exactly one library file"); | ||
| const [libraryDirent] = libraryDirents; | ||
| assert(libraryDirent.isFile(), "Expected a library file"); | ||
| const libraryPath = path.join(libraryDirent.parentPath, libraryDirent.name); |
Copilot
AI
May 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs.Dirent objects do not have a 'parentPath' property. Replace 'libraryDirent.parentPath' with the known directory path (likely 'archPath') to correctly construct the file path.
| const libraryPath = path.join(libraryDirent.parentPath, libraryDirent.name); | |
| const libraryPath = path.join(archPath, libraryDirent.name); |
025310b to
5ad833d
Compare
578e776 to
44f4200
Compare
Merging this PR will: