支持命令行中的配置覆盖配置文件中的配置 #5350
支持命令行中的配置覆盖配置文件中的配置 #5350
Conversation
支持命令行中的配置覆盖配置文件中的配置
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
internal/vite-config/src/utils/env.tsOops! Something went wrong! :( ESLint: 9.17.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe changes introduce a new utility function Changes
Sequence DiagramsequenceDiagram
participant Script as NPM Script
participant EnvUtils as Environment Utility
participant EnvConfig as Environment Configuration
Script->>EnvUtils: Invoke with lifecycle script
EnvUtils->>EnvUtils: Parse script parameters
EnvUtils->>EnvConfig: Merge parsed configurations
EnvConfig-->>EnvUtils: Updated environment variables
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/vite-config/src/utils/env.ts (1)
86-89: Improve logging and validation of command-line overrides.The current implementation has several areas for improvement:
- The log message is not internationalized
- Logging the entire configuration object could expose sensitive data
- Missing validation before merging configurations
Consider these improvements:
// 命令行中的配置 const scriptConfig = getConfByScript(); + // Filter out any non-matching keys before merge + Object.keys(scriptConfig).forEach((key) => { + if (!reg.test(key)) { + delete scriptConfig[key]; + } + }); + envConfig = { ...envConfig, ...scriptConfig }; - console.log('命令行中的参数已经覆盖配置文件中的参数 %o', scriptConfig); + if (Object.keys(scriptConfig).length > 0) { + console.log( + 'Command-line parameters have overridden the following keys:', + Object.keys(scriptConfig).join(', ') + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/vite-config/src/utils/env.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
🔇 Additional comments (1)
internal/vite-config/src/utils/env.ts (1)
Line range hint
32-90: Verify the impact on existing npm scripts.Since this change affects how command-line parameters are parsed, we should verify that it doesn't break existing npm scripts that might be using different parameter formats.
Let's check the existing npm scripts and their parameter formats:
✅ Verification successful
Parameter parsing change is compatible with existing npm scripts
The verification of npm scripts across the codebase shows that all existing scripts use simple parameter formats that are compatible with the implemented regex pattern. No scripts were found that would be affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all package.json files and extract scripts that use environment variables echo "Checking npm scripts that might be affected..." find . -name "package.json" -type f -exec sh -c ' echo "\nChecking: {}" jq -r ".scripts | to_entries | .[] | select(.value | contains(\"=\")) | .value" "{}" ' \;Length of output: 91643
| const regex = /(\w+)=(\w+)/g; | ||
| /** | ||
| * 获取命令行中传入的参数 | ||
| */ | ||
| function getConfByScript() { | ||
| const script = process.env.npm_lifecycle_script; | ||
| const scriptEnv: Record<string, string> = {}; | ||
| let matcher; | ||
| while (true) { | ||
| matcher = regex.exec(script as string); | ||
| if (matcher === null) { | ||
| break; | ||
| } | ||
| if (matcher.index === regex.lastIndex) { | ||
| regex.lastIndex++; | ||
| } | ||
|
|
||
| if (matcher) { | ||
| const key = matcher[1] as string; | ||
| scriptEnv[key] = matcher[2] as string; | ||
| } | ||
| } | ||
|
|
||
| return scriptEnv; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of command-line parameter parsing.
The current implementation has several limitations and potential issues:
- The regex pattern is too restrictive and doesn't support common npm script parameter formats
- The while(true) loop with regex.exec() could be problematic
- Contains redundant checks and lacks proper error handling
Consider this safer implementation:
-const regex = /(\w+)=(\w+)/g;
+const regex = /([A-Za-z][\w-]*)=(["']?)([^\n"']*)\2/g;
function getConfByScript() {
const script = process.env.npm_lifecycle_script;
+ if (!script) {
+ return {};
+ }
+
const scriptEnv: Record<string, string> = {};
- let matcher;
- while (true) {
- matcher = regex.exec(script as string);
- if (matcher === null) {
- break;
- }
- if (matcher.index === regex.lastIndex) {
- regex.lastIndex++;
- }
-
- if (matcher) {
- const key = matcher[1] as string;
- scriptEnv[key] = matcher[2] as string;
- }
+ for (const match of script.matchAll(regex)) {
+ const [, key, , value] = match;
+ scriptEnv[key] = value;
}
return scriptEnv;
}The improved version:
- Supports quoted values and hyphenated keys
- Uses for...of with matchAll() instead of while(true)
- Adds null check for npm_lifecycle_script
- Properly destructures regex groups
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const regex = /(\w+)=(\w+)/g; | |
| /** | |
| * 获取命令行中传入的参数 | |
| */ | |
| function getConfByScript() { | |
| const script = process.env.npm_lifecycle_script; | |
| const scriptEnv: Record<string, string> = {}; | |
| let matcher; | |
| while (true) { | |
| matcher = regex.exec(script as string); | |
| if (matcher === null) { | |
| break; | |
| } | |
| if (matcher.index === regex.lastIndex) { | |
| regex.lastIndex++; | |
| } | |
| if (matcher) { | |
| const key = matcher[1] as string; | |
| scriptEnv[key] = matcher[2] as string; | |
| } | |
| } | |
| return scriptEnv; | |
| } | |
| const regex = /([A-Za-z][\w-]*)=(["']?)([^\n"']*)\2/g; | |
| /** | |
| * 获取命令行中传入的参数 | |
| */ | |
| function getConfByScript() { | |
| const script = process.env.npm_lifecycle_script; | |
| if (!script) { | |
| return {}; | |
| } | |
| const scriptEnv: Record<string, string> = {}; | |
| for (const match of script.matchAll(regex)) { | |
| const [, key, , value] = match; | |
| scriptEnv[key] = value; | |
| } | |
| return scriptEnv; | |
| } |
支持命令行中的配置覆盖配置文件中的配置
Summary by CodeRabbit