-
Notifications
You must be signed in to change notification settings - Fork 1
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
--out
path is now{build}/{configuration}
in the CMake RN CLI. - Replaces
stripPathSuffix
boolean with apathSuffix
enum (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-suffix
when 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
pathSuffix
is set tostrip
oromit
, as both modes can cause collisions.
.action(async (pathArg, { force, prune, pathSuffix, android, apple }) => {
* - `"strip"` (default): Path gets stripped to its basename and the library name will be `my-pkg--my-addon`. | ||
* - `"keep"`: The full path is kept and the library name will be `my-pkg--build-Release-my-addon`. | ||
*/ | ||
pathSuffix?: "strip" | "omit" | "keep"; | ||
}; | ||
|
||
function assertOptions(opts: unknown): asserts opts is PluginOptions { |
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.
Copilot uses AI. Check for mistakes.
@@ -82,7 +82,7 @@ const cleanOption = new Option( | |||
const outPathOption = new Option( | |||
"--out <path>", | |||
"Specify the output directory to store the final build artifacts" | |||
); | |||
).default(false, "./{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.
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}"); |
Copilot uses AI. Check for mistakes.
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-rn
to save the final prebuilds into./{build}/{configuration}
(ex../build/Release
)./build/Release
prebuilds when transformingrequire("bindings")(...)
statements.build-Release
as a part of the library name.The naming strategy will now be controlled by a
pathSuffix
option passed to the Babel plugin (the following are the docs for the option):Or when linking through a
--path-suffix
option or theNODE_API_PATH_SUFFIX
environment variable.