fix: respect explicit wb env #552
Conversation
Summary of ChangesHello @exKAZUu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial fix to the environment variable loading mechanism within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
packages/wb/src/project.ts
Outdated
| const originalEnv = { WB_ENV: process.env.WB_ENV, NODE_ENV: process.env.NODE_ENV }; | ||
| const [envVars, envPathAndLoadedEnvVarCountPairs] = readEnvironmentVariables(this.argv, this.dirPath); | ||
| for (const [envPath, count] of envPathAndLoadedEnvVarCountPairs) { | ||
| console.info(`Loaded ${count} environment variables from ${envPath}`); | ||
| } | ||
| // Overwrite environment variables even though this behavior is non-standard | ||
| // because `bun wb ...` will load .env and .env.local before `wb` loads other variables. | ||
| return { ...process.env, ...envVars }; | ||
| const mergedEnv: Record<string, string | undefined> = { ...process.env, ...envVars }; | ||
| for (const [key, value] of Object.entries(originalEnv)) { | ||
| if (value !== undefined) { | ||
| // Keep explicitly provided environment selectors so cascading respects the caller's intention. | ||
| mergedEnv[key] = value; | ||
| } | ||
| } | ||
| return mergedEnv; |
There was a problem hiding this comment.
WB_ENV と NODE_ENV の値を復元するロジックは、より簡潔に記述できます。
中間オブジェクト originalEnv を作成してループ処理する代わりに、これらの特定の環境変数を直接確認して復元する方が、コードがより直接的になり、不要なオブジェクト作成とループをなくすことができます。これにより、コードの可読性と保守性が向上します。
| const originalEnv = { WB_ENV: process.env.WB_ENV, NODE_ENV: process.env.NODE_ENV }; | |
| const [envVars, envPathAndLoadedEnvVarCountPairs] = readEnvironmentVariables(this.argv, this.dirPath); | |
| for (const [envPath, count] of envPathAndLoadedEnvVarCountPairs) { | |
| console.info(`Loaded ${count} environment variables from ${envPath}`); | |
| } | |
| // Overwrite environment variables even though this behavior is non-standard | |
| // because `bun wb ...` will load .env and .env.local before `wb` loads other variables. | |
| return { ...process.env, ...envVars }; | |
| const mergedEnv: Record<string, string | undefined> = { ...process.env, ...envVars }; | |
| for (const [key, value] of Object.entries(originalEnv)) { | |
| if (value !== undefined) { | |
| // Keep explicitly provided environment selectors so cascading respects the caller's intention. | |
| mergedEnv[key] = value; | |
| } | |
| } | |
| return mergedEnv; | |
| const [envVars, envPathAndLoadedEnvVarCountPairs] = readEnvironmentVariables(this.argv, this.dirPath); | |
| for (const [envPath, count] of envPathAndLoadedEnvVarCountPairs) { | |
| console.info(`Loaded ${count} environment variables from ${envPath}`); | |
| } | |
| // Overwrite environment variables even though this behavior is non-standard | |
| // because `bun wb ...` will load .env and .env.local before `wb` loads other variables. | |
| const mergedEnv: Record<string, string | undefined> = { ...process.env, ...envVars }; | |
| // Keep explicitly provided environment selectors so cascading respects the caller's intention. | |
| if (process.env.WB_ENV !== undefined) { | |
| mergedEnv.WB_ENV = process.env.WB_ENV; | |
| } | |
| if (process.env.NODE_ENV !== undefined) { | |
| mergedEnv.NODE_ENV = process.env.NODE_ENV; | |
| } | |
| return mergedEnv; |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where explicitly provided WB_ENV and NODE_ENV environment variables were being overwritten by values from loaded .env files. The fix ensures that when these environment selectors are explicitly set (e.g., via command line or by command handlers), they are preserved after merging with file-loaded variables, allowing the environment cascading logic to respect the caller's intention.
Key Changes:
- Captures original
WB_ENVandNODE_ENVvalues before loading environment files - Restores these explicit values after merging with file-loaded variables to ensure cascading behavior respects caller intent
- Maintains backward compatibility by only restoring values that were explicitly defined
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/wb/src/project.ts
Outdated
| const originalEnv = { WB_ENV: process.env.WB_ENV, NODE_ENV: process.env.NODE_ENV }; | ||
| const [envVars, envPathAndLoadedEnvVarCountPairs] = readEnvironmentVariables(this.argv, this.dirPath); | ||
| for (const [envPath, count] of envPathAndLoadedEnvVarCountPairs) { | ||
| console.info(`Loaded ${count} environment variables from ${envPath}`); | ||
| } | ||
| // Overwrite environment variables even though this behavior is non-standard | ||
| // because `bun wb ...` will load .env and .env.local before `wb` loads other variables. | ||
| return { ...process.env, ...envVars }; | ||
| const mergedEnv: Record<string, string | undefined> = { ...process.env, ...envVars }; | ||
| for (const [key, value] of Object.entries(originalEnv)) { | ||
| if (value !== undefined) { | ||
| // Keep explicitly provided environment selectors so cascading respects the caller's intention. | ||
| mergedEnv[key] = value; | ||
| } | ||
| } | ||
| return mergedEnv; |
There was a problem hiding this comment.
This new behavior of preserving explicit WB_ENV and NODE_ENV values lacks test coverage. Consider adding tests to verify that:
- When WB_ENV or NODE_ENV are explicitly set in process.env, they are preserved after merging with loaded env variables
- When these variables are undefined in process.env, they can be overwritten by loaded env files
- The cascading behavior in
readEnvironmentVariablesrespects the explicitly provided values
This is critical functionality that affects environment variable resolution across the application.
Co-authored-by: WillBooster (Codex CLI) <[email protected]>
Close #
Self Check
All checks have passedon this page.WIP:.yarn test) passed.yarn lint) passed.