Default out path to {build}/{configuration} directory and refactored pathSuffix naming strategy#150
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.
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 { |
There was a problem hiding this comment.
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}"); |
There was a problem hiding this comment.
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.
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.