Conversation
WalkthroughChangesThe change adds platform-specific storage for environment-derived defines, routes Environment define inlining
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:12 AM PT - Jul 18th, 2026
❌ @robobun, your commit 5e61e64 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34560That installs a local version of the PR into your bun-34560 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
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/js_parser/visit/visit_expr.rs`:
- Around line 1403-1433: Extract the shared define substitution and DCE-flag
propagation from the preceding dots handling and the env_dots block into a
helper, then use it for both paths. Ensure the helper also propagates
method_call_must_be_replaced_with_undefined so both define sources remain
behaviorally aligned, while preserving each block’s existing lookup and
precedence behavior.
🪄 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
Run ID: 66163385-39b6-4464-88a1-d3aad82b2b40
📒 Files selected for processing (6)
src/bundler/defines.rssrc/bundler/options.rssrc/js_parser/lib.rssrc/js_parser/p.rssrc/js_parser/visit/visit_expr.rstest/bundler/bundler_env.test.ts
…-spawn Windows tests
There was a problem hiding this comment.
Thanks for addressing the optional-chain feedback — the e_.optional_chain.is_none() guard and the process.env?.X regression test cover it. Beyond the inline nit, I also checked that all three Define { .. } literal sites include env_dots, that e_index never consulted dots (so process.env["X"] behavior is unchanged), and that the referenced CaseInsensitiveAsciiStringArrayHashMap / starts_with_case_insensitive_ascii helpers exist. Leaving for a human to sign off given the user-visible precedence change (explicit --define now beats env-derived) and the overlap with #33857.
Extended reasoning...
The author's follow-up commit (5e61e64) addressed my prior review: the env_dots block is now gated on e_.optional_chain.is_none(), the lookup order was inverted to check is_dot_define_match before the map lookup, and a direct-spawn regression test asserts process.env?.X is not inlined. I verified the guard is present at visit_expr.rs:1407 and the test at bundler_env.test.ts:268-273. I also confirmed all three struct-literal construction sites of Define (defines.rs:259, options.rs:1459/1748) carry the new env_dots field, that e_index has no defines.dots lookup so bracket-access inlining behavior is unchanged by moving env entries out of dots, and that the Windows-only helpers referenced (bun_collections::CaseInsensitiveAsciiStringArrayHashMap, bun_core::strings::starts_with_case_insensitive_ascii) exist in-tree. Deferring rather than approving because this changes user-visible --define vs --env inline precedence on all platforms, adds a per-dot-expression check in the parser hot path, and subsumes open PR #33857 — a maintainer should confirm the precedence change and decide the fate of #33857.
|
CI: the only hard failure in build 75109 is |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Problem
On Windows,
bun build --env inline(andBun.build({ env: "inline" })) does not substituteprocess.env.PATHunless the source spells it exactlyprocess.env.Path, because Windows reports the variable asPathand the bundler's define lookup is case-sensitive.At runtime
process.env.PATH,process.env.Pathandprocess.env.pathall return the same value on Windows, so the inline should too. The same applies to--env PREFIX_*: the prefix was matched case-sensitively against the stored key, so--env SYS*never matchedSystemRoot.This is what makes
test/bundler/bundler_env.test.ts > env/inline systemfail on Windows CI (it prints the runtime PATH instead of the bundle-time value).Cause
copy_env_for_defineiterates the env loader's stored keys (which keep the OS case on Windows) and builds define keys likeprocess.env.Path. Those land inDefine.dots, which is a case-sensitiveStringHashMapkeyed by the last segment, soprocess.env.PATHin source never matches.Fix
Definegains a dedicatedenv_dotsmap for env-derivedprocess.env.Xentries, keyed by the env-var name alone. On Windows it is aCaseInsensitiveAsciiStringArrayHashMap; on other platforms it is the regular case-sensitiveStringArrayHashMap. Thee_dotvisitor consultsenv_dotsafter the existingdotslookup, guarded onoptional_chain.is_none()soprocess.env?.Xis left alone (matching thedotspath), and verifies the target is an unboundprocess.envviais_dot_define_matchbefore hashing the name intoenv_dots.is_dot_define_matchis made generic overAsRef<[u8]>so the["process","env"]prefix can be passed as a const slice.copy_env_for_definenow matches the--env PREFIX_*prefix case-insensitively on Windows.A side effect of routing env-derived entries through a separate map checked after
dots: an explicit--define process.env.X=...now wins over an env-derived value for the same key. That matches the existingprocess.env.NODE_ENVinsert-only-if-absent handling and is the behaviour #33857 is after.Verification
New tests in
test/bundler/bundler_env.test.ts:bundler/env via spawn > process.env.X matches the env-var name case-insensitively on Windows only: spawnsbun build --env inlinedirectly (bypassesitBundled, so it registers on Windows today). On Windows all three casings inline; on POSIX only the exact case. Fails on released Windows, passes with the fix.bundler/env via spawn > --env PREFIX_* matches case-insensitively on Windows only: same pattern for prefix mode. Fails on released Windows, passes with the fix.bundler/env via spawn > process.env?.X is not inlined by --env inline: regression guard that the optional-chain form stays untouched.env/inline-explicit-define-wins: explicit--definebeats env-derived. Fails on the released binary on all platforms, passes with the fix.env/inline-env-var-name-caseandenv/prefix-env-var-name-case:itBundledequivalents of the first two (need test/bundler: stop silently dropping every itBundled test on Windows #34552 to register on the Windows lanes).All 13
bundler_env.test.tstests pass with the fix on linux-x64 (1 fails on the released binary) and windows-x64 (2 of the 3 direct-spawn tests fail on the released binary).bundler_edgecase,bundler_string,bundler_browser,bundler_bun,bundler_cjs2esmandcli/run/env.test.tspass unchanged.cargo check -p bun_js_parser -p bun_bundler --target x86_64-pc-windows-msvcis clean.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_env.test.ts