-
Notifications
You must be signed in to change notification settings - Fork 3
Update Ferric to expect napi.rs@3
#114
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
|
napi.rs@v3
napi.rs@3
1df8b75
to
2c1db25
Compare
const libraryName = determineLibraryBasename([ | ||
...androidLibraries.map(([, outputPath]) => outputPath), | ||
...appleLibraries.map(([, outputPath]) => outputPath), | ||
]); |
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.
Moving this down to include the appleLibraries
and supporting --apple
without --android
to build the TypeScript declaration file.
packages/ferric/src/napi-rs.ts
Outdated
// Workaround to instruct the rust compiler to not worry about undefined symbols | ||
// TODO: Figure out why this is actually necessary - it should link against sys::nodeapi | ||
process.env.RUSTFLAGS = | ||
"-C link-arg=-undefined -C link-arg=dynamic_lookup"; |
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.
I haven't looked into this more, but it troubles me that we have to allow undefined symbols this way 🤔
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.
I deleted this again, the example builds without it 🤔 I have no idea why it was needed at that point in time 😕
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 updates Ferric to target napi.rs v3 by switching from the spawn-based CLI invocation to the new NapiCli API and updating dependency versions.
- Replaces the old spawn usage with NapiCli.build in generating TypeScript declarations.
- Consolidates the generation of TypeScript declarations and entrypoint for both Android and Apple libraries.
- Bumps dependency versions in package.json and Cargo.toml to align with napi.rs v3.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/ferric/src/napi-rs.ts | Migrates from using spawn to invoke the CLI to using the NapiCli.build API with environment adjustments. |
packages/ferric/src/build.ts | Consolidates declaration/entrypoint generation to include both android and apple libraries. |
packages/ferric/package.json | Updates the @napi-rs/cli dependency from v2 to v3.0.0-alpha.80. |
packages/ferric-example/Cargo.toml | Updates napi and napi-derive dependency versions to v3.0.0-alpha and adjusts build profile settings. |
Comments suppressed due to low confidence (2)
packages/ferric/src/napi-rs.ts:55
- Modifying process.env globally may lead to race conditions if generateTypeScriptDeclarations is called concurrently; consider isolating or synchronizing this change.
process.env.RUSTFLAGS = "-C link-arg=-undefined -C link-arg=dynamic_lookup";
packages/ferric/src/build.ts:225
- [nitpick] Since libraryName is derived from both android and apple library outputs, consider renaming it to more clearly convey its combined nature.
const libraryName = determineLibraryBasename([ ...androidLibraries.map(([, outputPath]) => outputPath), ...appleLibraries.map(([, outputPath]) => outputPath) ]);
2c1db25
to
b05c42a
Compare
As v3 of
napi.rs
is maturing, we should target this instead of v2.