-
Notifications
You must be signed in to change notification settings - Fork 8
Add plain mode for inject-site-configs #3786
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 introduces a new option to the CLI for injecting site configurations in a plain JSON format and adds corresponding update documentation.
- Adds the "--plain" option to output plain JSON encoding without escaping backslashes
- Updates changeset documentation to reflect the new feature
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/cli/src/commands/site-configs.ts | Implements a new "--plain" option for injecting site configs |
.changeset/seven-knives-trade.md | Adds a changeset for the plain encoding feature |
@@ -53,6 +54,9 @@ export const injectSiteConfigsCommand = new Command("inject-site-configs") | |||
return substr; | |||
} | |||
const str = replacerFunctions[type](siteConfigs, env); | |||
if (options.plain) { | |||
return JSON.stringify(str); |
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 plain mode option currently returns JSON.stringify(str), which escapes backslashes contrary to its description. To truly output plain JSON without escaping backslashes, consider returning the raw value (e.g. str) or adjust the logic to meet the intended behavior.
return JSON.stringify(str); | |
return str; |
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.
@fraxachun is it possible that Copilot is correct here?

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.
Maybe we should remove the string replace in L63 instead? Why do we need this?
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.
@fraxachun what's the status here? Is this still needed?
I will close it as --plain should be the default mode, workarounds like these should never have made it to the code. We currently work around the workaround in our internal cli package so we don't need this PR anymore. Nevertheless, I created a PR for next. |
No description provided.