-
Notifications
You must be signed in to change notification settings - Fork 4
Fix expansion of options in --build and --out
#278
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
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 fixes the expansion of template strings in --build and --out CLI options for cmake-rn, allowing users to reference other option values using {placeholder} syntax.
Key Changes:
- Adds a
expandTemplatefunction to resolve{placeholder}references in option values - Sets explicit defaults for
--buildand--outoptions that use template syntax - Removes the deprecated
getBuildPathhelper in favor of direct template expansion
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cmake-rn/src/cli.ts | Implements template expansion logic, updates option defaults, and removes legacy path resolution helper |
| .changeset/fresh-frogs-enter.md | Documents the fix as a patch-level change |
| values: Record<string, unknown>, | ||
| ): string { | ||
| return input.replaceAll(/{([^}]+)}/g, (_, key: string) => | ||
| typeof values[key] === "string" ? values[key] : "", |
Copilot
AI
Oct 21, 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 type guard typeof values[key] === 'string' only handles string values, but the values parameter is typed as Record<string, unknown>. Non-string values like numbers or booleans will be silently replaced with empty strings. Consider converting non-string values to strings using String(values[key]) or documenting why only strings should be expanded.
| typeof values[key] === "string" ? values[key] : "", | |
| String(values[key]), |
packages/cmake-rn/src/cli.ts
Outdated
| process.cwd(), | ||
| expandTemplate(baseOptions.out, baseOptions), | ||
| ); | ||
| const { verbose, clean, source, build: buildPath } = baseOptions; |
Copilot
AI
Oct 21, 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 destructuring extracts verbose, clean, and source but these variables are unused in the immediate scope. Only buildPath is actually used. Consider removing unused destructured variables or moving this destructuring closer to where all variables are needed.
| const { verbose, clean, source, build: buildPath } = baseOptions; | |
| const { build: buildPath } = baseOptions; |
a9857c6 to
753c553
Compare
ece6163 to
cf53c58
Compare
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| return input.replaceAll(/{([^}]+)}/g, (_, key: string) => | ||
| typeof values[key] === "string" ? values[key] : "", | ||
| ); |
Copilot
AI
Oct 21, 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 template expansion returns an empty string for non-string values, which could silently produce incorrect paths. For example, if {configuration} is undefined or non-string, the resulting path would be malformed (e.g., build//artifacts). Consider throwing an error for missing or non-string values, or at minimum documenting this behavior.
| return input.replaceAll(/{([^}]+)}/g, (_, key: string) => | |
| typeof values[key] === "string" ? values[key] : "", | |
| ); | |
| return input.replaceAll(/{([^}]+)}/g, (_, key: string) => { | |
| if (typeof values[key] !== "string") { | |
| throw new Error(`Template variable '{${key}}' is missing or not a string (got: ${typeof values[key]})`); | |
| } | |
| return values[key] as string; | |
| }); |
| ); | ||
| baseOptions.out = path.resolve( | ||
| process.cwd(), | ||
| expandTemplate(baseOptions.out, baseOptions), |
Copilot
AI
Oct 21, 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 out option expansion on line 171 references {build} in its default value ({build}/{configuration}), but the template is expanded using the original baseOptions.build string (e.g., {source}/build) rather than the already-resolved build path. This creates a circular dependency issue where out will contain the literal string {source}/build/{configuration} instead of the resolved path. The expansion of out should use the already-resolved build value.
| expandTemplate(baseOptions.out, baseOptions), | |
| expandTemplate( | |
| baseOptions.out, | |
| { ...baseOptions, build: baseOptions.build } | |
| ), |
| function expandTemplate( | ||
| input: string, | ||
| values: Record<string, unknown>, | ||
| ): string { |
Copilot
AI
Oct 21, 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 expandTemplate function lacks documentation explaining the template syntax, expected placeholders (e.g., {source}, {build}, {configuration}), and behavior when values are missing or non-string. Add a JSDoc comment describing the template format and edge case handling.
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.
🤟
Merging this PR will:
--buildand--outofcmake-rn.