Repository navigation
fix(bundler): honor env: disable for process.env.NODE_ENV - #35954
baima365-web wants to merge 1 commit into
Conversation
Bun.build({ env: "disable" }) was still folding process.env.NODE_ENV to
the build-time value. The bracket form process.env["NODE_ENV"] stayed
dynamic but dot form was incorrectly inlined.
Fix: Check env_behavior before injecting NODE_ENV define.
Fixes oven-sh#35952
WalkthroughThe bundler now avoids injecting ChangesEnvironment variable inlining
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/options.rs`:
- Around line 921-925: Update the environment-injection condition in
defines_from_transform_options to use the local behavior value instead of the
undefined self.env_behavior reference, comparing it against
api::DotEnvBehavior::disable. Preserve the existing NODE_ENV and BUN_ENV define
insertion behavior when dotenv handling is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 563d4b96-fe24-41f2-b576-3b3bb18b7016
📒 Files selected for processing (1)
src/bundler/options.rs
| // Only inject NODE_ENV if env_behavior is not Disable | ||
| if self.env_behavior != DotEnv::Behavior::Disable { | ||
| user_defines.get_or_put_value(b"process.env.NODE_ENV", quoted_node_env.clone())?; | ||
| user_defines.get_or_put_value(b"process.env.BUN_ENV", quoted_node_env)?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="$(pwd)/src/bundler/options.rs"
rg -n 'pub fn defines_from_transform_options|self\.env_behavior|let mut behavior|DotEnvBehavior::disable' "$file"Repository: oven-sh/bun
Length of output: 487
🏁 Script executed:
#!/bin/bash
set -eu
file="$(pwd)/src/bundler/options.rs"
echo "Function signature and dotenv behavior handling:"
sed -n '815,930p' "$file" | cat -n | sed 's/^/options.rs /'
echo
echo "Top-level module/type aliases around this file:"
rg -n '^pub (use|mod) |type DotEnv =|enum DotEnvBehavior|struct DotEnv|api::' "$file" | sed -n '1,80p'Repository: oven-sh/bun
Length of output: 10670
Fix the out-of-scope self reference.
defines_from_transform_options is a free function, so self.env_behavior is undefined and prevents compilation. Use the local behavior value and the existing api::DotEnvBehavior::disable variant.
Proposed fix
- // Only inject NODE_ENV if env_behavior is not Disable
- if self.env_behavior != DotEnv::Behavior::Disable {
+ // Only inject NODE_ENV and BUN_ENV when dotenv behavior is not Disable
+ if behavior != api::DotEnvBehavior::disable {📝 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.
| // Only inject NODE_ENV if env_behavior is not Disable | |
| if self.env_behavior != DotEnv::Behavior::Disable { | |
| user_defines.get_or_put_value(b"process.env.NODE_ENV", quoted_node_env.clone())?; | |
| user_defines.get_or_put_value(b"process.env.BUN_ENV", quoted_node_env)?; | |
| } | |
| // Only inject NODE_ENV and BUN_ENV when dotenv behavior is not Disable | |
| if behavior != api::DotEnvBehavior::disable { | |
| user_defines.get_or_put_value(b"process.env.NODE_ENV", quoted_node_env.clone())?; | |
| user_defines.get_or_put_value(b"process.env.BUN_ENV", quoted_node_env)?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bundler/options.rs` around lines 921 - 925, Update the
environment-injection condition in defines_from_transform_options to use the
local behavior value instead of the undefined self.env_behavior reference,
comparing it against api::DotEnvBehavior::disable. Preserve the existing
NODE_ENV and BUN_ENV define insertion behavior when dotenv handling is enabled.
|
Thanks for the PR. This is the same bug as #35952 (opened a few hours earlier, with a test for both the JS API and the CLI backend), so I'm closing this one as a duplicate and keeping #35952 as the fix. The underlying reports are #19508, #20183 and #22820. Two notes on this diff, for reference:
|
What does this PR do?
Bun.build({ env: "disable" })was still foldingprocess.env.NODE_ENV(dot form) to the build-time value, while the bracket formprocess.env["NODE_ENV"]stayed dynamic. The CLIbun build --env disablealready left both forms dynamic.Problem
Fix
Check
env_behaviorbefore injecting NODE_ENV define indefines_from_transform_options. Whenenv_behaviorisDisable, skip the NODE_ENV and BUN_ENV defines entirely.Fixes #35952