-
Notifications
You must be signed in to change notification settings - Fork 7
Default out path to {build}/{configuration} directory and refactored pathSuffix naming strategy
#150
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
|
db793b1 to
3d1aaeb
Compare
|
I was contemplating actually interpreting the |
3d1aaeb to
6ce9b9d
Compare
{build}/{configuration} directory{build}/{configuration} directory and refactored "path suffix" naming strategies
{build}/{configuration} directory and refactored "path suffix" naming strategies{build}/{configuration} directory and refactored pathSuffix naming strategy
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
Aligns the default output directory with cmake-js (./build/{configuration}) and refactors the naming strategy for Node-API modules to use a pathSuffix option instead of a boolean flag.
- Default
--outpath is now{build}/{configuration}in the CMake RN CLI. - Replaces
stripPathSuffixboolean with apathSuffixenum (omit/strip/keep) across host CLI, Babel plugin, and path-utils. - Updates tests, Podspec, and scripts to reflect the new naming strategy and output defaults.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| path-utils.ts | Swapped stripPathSuffix for pathSuffix and updated getLibraryName logic. |
| path-utils.test.ts | Expanded tests for strip, keep, and omit path suffix modes. |
| cli/program.ts | Replaced the old --strip-path-suffix option with --path-suffix. |
| cli/options.ts | Added pathSuffixOption with validation against `"strip" |
| babel-plugin/plugin.ts | Updated plugin to consume pathSuffix and added findNodeAddonForBindings. |
| babel-plugin/plugin.test.ts | Added comprehensive tests for all pathSuffix modes in both require and bindings. |
| react-native-node-api.podspec | Removed legacy --strip-path-suffix logic from the Podspec. |
| package.json | Added --out flag to weak-node-api build scripts. |
| cmake-rn/src/cli.ts | Introduced outPathOption and defaulted globalContext.out to <build>/<configuration>. |
Comments suppressed due to low confidence (2)
packages/host/react-native-node-api.podspec:9
- The Podspec no longer exposes a way to pass
--path-suffixwhen copying frameworks. Consider re-adding logic to readNODE_API_PATH_SUFFIX(or default) and include--path-suffix <strategy>in the CLI invocation.
COPY_FRAMEWORKS_COMMAND ||= "#{CLI_COMMAND} link --apple '#{Pod::Config.instance.installation_root}'"
packages/host/src/node/cli/program.ts:71
- [nitpick] Previously the CLI warned when stripping suffixes to avoid naming collisions. You may want to reintroduce a warning when
pathSuffixis set tostriporomit, as both modes can cause collisions.
.action(async (pathArg, { force, prune, pathSuffix, android, apple }) => {
| pathSuffix?: "strip" | "omit" | "keep"; | ||
| }; | ||
|
|
||
| function assertOptions(opts: unknown): asserts opts is PluginOptions { |
Copilot
AI
Jul 6, 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.
assertOptions still only checks stripPathSuffix and does not validate the new pathSuffix property. Update it to assert that opts.pathSuffix, if present, is one of 'strip', 'keep', or 'omit', and remove any leftover stripPathSuffix branches.
| "--out <path>", | ||
| "Specify the output directory to store the final build artifacts" | ||
| ); | ||
| ).default(false, "./{build}/{configuration}"); |
Copilot
AI
Jul 6, 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.
The .default(false, "./{build}/{configuration}") call will set the default to false rather than the intended path string. Use .default("./{build}/{configuration}") so --out defaults correctly.
| ).default(false, "./{build}/{configuration}"); | |
| ).default("./{build}/{configuration}"); |
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.
This is on purpose, since we need the values of build and configuration to construct the default value, we're setting it to false to defer defining its actual value.
6ce9b9d to
883978b
Compare
|
I need this merged to progress on the example repo, please review in retrospect 🙏 |
This is to align with the behavior of "cmake-js" to address some of the concerns raised in #146.
Merging this PR will:
cmake-rnto save the final prebuilds into./{build}/{configuration}(ex../build/Release)./build/Releaseprebuilds when transformingrequire("bindings")(...)statements.build-Releaseas a part of the library name.The naming strategy will now be controlled by a
pathSuffixoption passed to the Babel plugin (the following are the docs for the option):Or when linking through a
--path-suffixoption or theNODE_API_PATH_SUFFIXenvironment variable.