Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new --out-to-build CLI flag so that artifacts are placed under build/<configuration>, mirroring cmake-js and node-gyp, and renames the existing outPathOption variable while adding logic to ensure --out and --out-to-build are mutually exclusive.
- Introduce
--out-to-buildoption incli.ts - Rename
outPathOptionvariable tooutOption - Add runtime assertion to prevent using
--outwith--out-to-build
Comments suppressed due to low confidence (2)
packages/cmake-rn/src/cli.ts:82
- [nitpick] Renaming
outPathOptiontooutOptionreduces clarity and consistency. Consider reverting tooutPathOptionor usingoutDirOptionto more clearly describe its purpose.
const outOption = new Option(
packages/cmake-rn/src/cli.ts:178
- There are no tests verifying the new
--out-to-buildbehavior or its conflict with--out. Adding unit tests for both the default and conflict scenarios will help prevent regressions.
if (globalContext.outToBuild) {
| } | ||
| } | ||
|
|
||
| if (globalContext.outToBuild) { |
There was a problem hiding this comment.
Instead of manually asserting mutual exclusion at runtime, use Commander.js's .conflicts() API to declare that --out and --out-to-build cannot be used together, providing built-in CLI validation and clearer error messaging.
There was a problem hiding this comment.
We'll want to make this the default behavior and as such, I expect it to just end up as default for the --out parameter.
cf4d3b7 to
c18a1be
Compare
|
Closing as superseded by #150 |
As a quick fix to address the issue of the output directory being misaligned with
cmake-jsandnode-gyp(mentioned in #146).Merging this PR will:
--out-to-buildoption which will output the prebuild artifact in thebuilddirectory (under "build/Release" or "build/Debug"), just likecmake-jsandnode-gypdoes.We might want to make this the default behaviour at some point.