-
Notifications
You must be signed in to change notification settings - Fork 7
Separate weak-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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
5b1fb6e to
7ccdc2d
Compare
46488d6 to
b07067e
Compare
b07067e to
0748c90
Compare
|
Finally - 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 the weak-node-api functionality from the host package into a dedicated weak-node-api package, addressing issue #260. The change promotes better modularity and establishes find_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 |
0748c90 to
81ffa78
Compare
81ffa78 to
46d5be1
Compare
…talled by the app
46d5be1 to
f46c595
Compare
This fixes #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 in Explicitweak-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 I think is desirable?