bun run: let env NODE_ENV=development override tsconfig jsx dev/prod - #36469
bun run: let env NODE_ENV=development override tsconfig jsx dev/prod#36469robobun wants to merge 4 commits into
Conversation
configure_defines() sampled is_production from the env loader directly but derived is_development only from define.dots, which bun run never populates (its LoadAllWithoutInlining env behavior skips the NODE_ENV define injection). The result was one-directional: env NODE_ENV=production forced the production JSX runtime over tsconfig react-jsxdev, but env NODE_ENV=development could not force the dev runtime over tsconfig react-jsx. Sample is_development from the env loader the same way is_production is. When the define map does carry a NODE_ENV (via --define, or on the bun build path), it still takes precedence and now clears the opposite flag so --define=production continues to beat env=development.
WalkthroughChangesThe transpiler now derives development mode from loaded NODE_ENV-driven JSX selection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced with Review feedback addressed in c86d65a (test title placeholder) and 19d4918 (dropped comment). CI on the first push was unrelated pre-existing/flaky lanes only; the new push re-runs it. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The 3-placeholder format string bound the trailing -> %s to the tsconfig value instead of the expected output.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/transpiler.rs`:
- Around line 579-588: Update the explicit NODE_ENV handling in the transpiler
mode-selection logic to reset is_production and is_development before matching
the defined value, so values other than "production" or "development" do not
inherit environment-derived flags. Preserve the existing matching behavior and
add coverage for an environment production value overridden by an explicit
"test" define, ensuring tsconfig controls JSX mode.
In `@test/bundler/transpiler/jsx-tsconfig-react-jsx.test.ts`:
- Line 36: Update the comment in the test case to remove the
implementation-history text “was: tsconfig won -> prod”, retaining only the
durable “env wins” invariant.
🪄 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: c4a58587-7260-4e49-8da8-df08b7f9fbd9
📒 Files selected for processing (2)
src/bundler/transpiler.rstest/bundler/transpiler/jsx-tsconfig-react-jsx.test.ts
There was a problem hiding this comment.
LGTM — all prior feedback (test title placeholder, comment noise, history note) is addressed and the bug hunter found nothing new this run.
What was reviewed:
is_developmentis seeded fromenv_loader.get_node_env(), symmetric with the existingis_productionline; both use the sameBUN_ENV→NODE_ENVfallback so the test'sBUN_ENVaxis is exercised.- Mutual-exclusion clears in the define-map branch prevent both flags being true; from env alone they can't both be true since they read the same var.
- The
--define="test"+ env leak-through is now symmetric with pre-existing prod-side behavior (line 544); scoped out to #36269 as discussed. - Tests: concurrent, drain pipes via
Promise.all, assert combined{stderr, stdout, exitCode},bunEnvspread with both env vars cleared before setting the one under test.
Extended reasoning...
Overview
Two-file change: src/bundler/transpiler.rs (configure_defines()) seeds is_development from the env loader alongside the existing is_production seed, and adds mutual-exclusion clears when the define-map override fires. test/bundler/transpiler/jsx-tsconfig-react-jsx.test.ts gains a 2×4 env-override matrix (NODE_ENV/BUN_ENV × both tsconfig jsx values × both env values) plus two --define-over-env precedence cases.
Security risks
None. This adjusts which JSX runtime import path (jsx-runtime vs jsx-dev-runtime) the transpiler selects based on already-trusted env/define inputs. No untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Medium — it's a user-facing transpiler behavior change, but a narrowly targeted symmetry fix. Before this PR, env NODE_ENV=production overrode tsconfig react-jsxdev but env NODE_ENV=development did not override tsconfig react-jsx; this makes both directions behave the same. The mechanism (read env loader directly, since LoadAllWithoutInlining skips injecting env into define.dots) is well-explained and matches what I read at transpiler.rs:544-600 and env_loader.rs:176-182.
Other factors
- All three prior review threads (my two nits + comment-cop) are resolved by commits c86d65a, 19d4918, ad86bb4; CodeRabbit's "reset both flags" suggestion was withdrawn after the author correctly showed it would regress the very case being fixed (
had_explicit_node_envis true for env-only, anddefine.dotshas no entry underLoadAllWithoutInlining). - Fail-before/pass-after was verified by the author (2 of 12 cases fail on main).
- The PR flags overlap with #36269 (same block,
force_node_envguard) as a trivial rebase — worth noting for whoever lands second, but not a blocker here. - Tests follow harness conventions:
tempDir,test.concurrent.each,{...bunEnv, NODE_ENV: undefined, BUN_ENV: undefined, [envVar]: value}, concurrent pipe drain, combined-object assertion.
1 participant
What
Since #34422 made tsconfig
"jsx": "react-jsx"select the production automatic runtime,bun runapplies envNODE_ENVto that selection asymmetrically:--define process.env.NODE_ENV='"development"'did force dev; only the env-var path was one-directional.Repro
Cause
configure_defines()samplesis_productionfrom the env loader directly (env_loader.is_production()) but derivesis_developmentonly fromoptions.define.dots["NODE_ENV"].bun runsetsenv.behavior = LoadAllWithoutInlining, which makesload_definesskip injecting envNODE_ENVinto the define map, sois_developmentcan never becometruefrom the environment.--definegoes into the define map regardless of behavior, which is why that path worked.Fix
Sample
is_developmentfrom the env loader alongsideis_production. When the define map does carry aNODE_ENVvalue (via--define, or on thebun buildpath which does inject it), that value still takes precedence and now clears the opposite flag, so--define process.env.NODE_ENV='"production"'continues to beat envNODE_ENV=development.This gives the precedence documented on
Pragma::development(--define>NODE_ENV> tsconfig) forbun runin both directions.Verification
Added a 2x4 matrix (
NODE_ENV/BUN_ENVx both tsconfig values x both env values) plus two--define-over-env cases totest/bundler/transpiler/jsx-tsconfig-react-jsx.test.ts. 2 of the 10 new cases fail on main ("react-jsx"+{NODE_ENV,BUN_ENV}=development), all pass with the fix.Existing suites green:
bundler_jsx,jsx-production,esbuild/tsconfig,bundler_env,bundler_cjs2esm -t NodeEnv,bundler_bun.Related
#36269 fixes the
bun build(bundled) half of the dev/prod precedence and touches the sameconfigure_definesblock with aforce_node_env == Unspecifiedguard andforce_node_env = Productionassignment; both read the sameis_production/is_developmentlocals so whichever lands second is a trivial rebase. #36209 and #36261 change theArguments.rs--jsx-*default and are independent of this change.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file