- Notifications
You must be signed in to change notification settings - Fork7
Separateweak-node-api package#308
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
5b1fb6e to7ccdc2dCompare46488d6 tob07067eCompareb07067e to0748c90CompareFinally - on the 5th (!!!) attempt, this passed without the Android emulator failing to launch or Gradle crashing. I'm suspecting the runner is running out-of-memory 🙃 |
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 separates theweak-node-api functionality from the host package into a dedicatedweak-node-api package, addressing issue#260. The change promotes better modularity and establishesfind_package as the recommended CMake integration pattern.
Key Changes:
- Created new
packages/weak-node-apipackage with its own build configuration, scripts, and TypeScript project structure - Updated all CMake integrations to use
find_package(weak-node-api REQUIRED CONFIG)instead ofinclude(${WEAK_NODE_API_CONFIG}) - Reorganized dependencies so that
hostand other packages import from the newweak-node-apipackage
Reviewed Changes
Copilot reviewed 38 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/weak-node-api/* | New package containing weak-node-api library, build scripts, CMake config, and TypeScript sources |
| packages/host/package.json | Removed weak-node-api build scripts and added dependency on new weak-node-api package |
| packages/host/src/node/weak-node-api.ts | Deleted (functionality moved to weak-node-api package) |
| packages/host/android/build.gradle | Updated to reference weak-node-api from node_modules and use find_package |
| packages/host/android/CMakeLists.txt | Simplified to use find_package instead of manual library configuration |
| packages/host/scripts/generate-weak-node-api-injector.mts | Updated to import from weak-node-api package |
| packages/cmake-rn/src/weak-node-api.ts | Updated to import paths from weak-node-api package |
| packages/ferric/src/cargo.ts | Updated to import prebuild paths from weak-node-api package |
| packages/gyp-to-cmake/src/transformer.ts | Updated to use find_package instead of include |
| apps/test-app/package.json | Added weak-node-api as a dependency |
| .github/workflows/check.yml | Updated build commands to target weak-node-api workspace |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
0748c90 to81ffa78Compare81ffa78 to46d5be1Compare46d5be1 tof46c595Compare60fae96 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Thisfixes#260 by moving the weak-node-api specifics from the host package into a separate package of the mono-repo.
Merging this PR will:
weak-node-apiand its related package scripts frompackages/hostintopackages/weak-node-apiand update the host to link against it.find_packagefunction as the recommended way of importing theweak-node-apitarget, over theinclude(...)pattern introduced inExplicitweak-node-apilinkage #249.weak-node-apipackage to the test app. This is the biggest drawback of a separate package in my opinion - and the decision is driven by a few limiting factors:vendored_frameworks(it's located using a glob expression relative to the pod root).:pathcan be used in the app's Podfile but not in a podspec, so we cannot simply locate and reference theweak-node-apipod "manually".TODO
weak-node-apipackage.weak-node-apipodspec by path instead of relying on React Native autolinking to find it. This will mean we can get rid of the test app's dependency onweak-node-apiand rely on a direct dependency from the host package - which Ithink is desirable?