cli: stop --jsx-* flags from switching the automatic JSX runtime to production - #36209
cli: stop --jsx-* flags from switching the automatic JSX runtime to production#36209robobun wants to merge 6 commits into
Conversation
WalkthroughCLI JSX override handling now preserves the JSX development setting instead of resetting it. A concurrent bundler test suite covers flag combinations, environment values, optional ChangesJSX runtime mode
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:41 PM PT - Jul 28th, 2026
❌ @robobun, your commit e481288 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36209That installs a local version of the PR into your bun-36209 --bun |
There was a problem hiding this comment.
LGTM — the two-line default flip is correct and now matches Pragma::default(), bunfig's jsx_dev = true, and JSBundler; NODE_ENV=production still reaches set_production() on the run path. The inline nit about test-file placement is non-blocking.
Extended reasoning...
Overview
Two-line change in src/runtime/cli/Arguments.rs: when any --jsx-* CLI flag is present and an api::Jsx is constructed, development now defaults to true (fresh-construct branch) or preserves prev.development (bunfig-merge branch) instead of hardcoding false. A new 12-case parameterized test in test/bundler/transpiler/jsx-cli-flags.test.ts spawns bun a.jsx against a shim react package that reports which runtime entrypoint was imported, across --jsx-import-source / --jsx-fragment / --jsx-factory / --jsx-runtime × NODE_ENV ∈ {unset, development, production}.
Correctness verification
I traced the claim end-to-end:
Pragma::default()atsrc/options_types/jsx.rs:201setsdevelopment: true.Pragma::from_apiatjsx.rs:329copiesjsx.developmentverbatim, so the oldfalsehere overrode the default only when a flag was present — exactly the reported inconsistency.- Bunfig's parser (
src/bunfig/bunfig.rs:953) defaultsjsx_dev = trueand only setsfalseforjsx = "react-jsx"; the merge branch now preserves that viaprev.developmentinstead of clobbering it. JSBundleratsrc/runtime/api/JSBundler.rs:191also defaultsdevelopment: true.NODE_ENV=productionstill flips to prod viaset_production(true)insrc/bundler/transpiler.rs:734/786, which the test'sproductioncases cover.
The finder-raised concern that the merge branch still overwrites prev.side_effects was checked and is pre-existing behavior unrelated to this fix.
Security risks
None. This is transpiler-option defaulting; no untrusted input parsing, no auth/crypto/fs surface.
Level of scrutiny
Low-to-moderate. A two-line default change in CLI arg assembly with a well-argued root cause and three independent in-tree precedents for the new value. The test is hermetic (tempDir, shim package, port-free), drains stdout/stderr/exited concurrently, spreads bunEnv, and asserts exact JSON — it follows harness conventions.
Other factors
The only finding is a nit about test-file placement (new jsx-cli-flags.test.ts vs. appending to jsx-production.test.ts, which the PR description still names). That's organizational and was a deliberate follow-up commit; it doesn't affect correctness. The comment-cop bot's earlier note about an explanatory code comment was addressed in d8f05e4.
…ction Passing any --jsx-* CLI flag (--jsx-import-source, --jsx-fragment, --jsx-factory, --jsx-runtime) populated the api::Jsx struct with a hardcoded development=false. With no flags, Pragma::default() has development=true, so adding an unrelated flag like --jsx-import-source=react silently flipped bun run from react/jsx-dev-runtime (jsxDEV) to react/jsx-runtime (jsx). Default to development=true (matching Pragma::default(), bunfig, and JSBundler) and preserve the prior bunfig value instead of clobbering it. NODE_ENV=production still wins via set_production(), same as before.
The existing jsx-production.test.ts describe.concurrent block spawns a heavy fixture (react-dom render + Bun.build + dynamic import) that races the 5s default timeout under debug+ASAN. Keeping the new --jsx-* CLI flag tests in a separate file gives them a clean pass/fail independent of that pre-existing timing.
d8f05e4 to
1ac228a
Compare
Add bunfig.toml cases (empty, jsx=react-jsx, jsx=react-jsxDEV) so the else arm at Arguments.rs:1467 that now reads prev.development is exercised. Reverting that line alone fails three of the new cases.
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 `@test/bundler/transpiler/jsx-cli-flags.test.ts`:
- Around line 26-49: Expand the `cases` matrix to add explicit development and
production entries for `--jsx-factory=h`, an explicit development entry for
`--jsx-runtime=automatic`, and separated-argument forms such as
`["--jsx-factory", "h"]` with both relevant environment outcomes. Preserve the
expected DEV/PROD behavior established by the existing cases.
🪄 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: eab96ecd-b9ed-4950-912d-9bd0b16d7a77
📒 Files selected for processing (2)
src/runtime/cli/Arguments.rstest/bundler/transpiler/jsx-cli-flags.test.ts
There was a problem hiding this comment.
LGTM — the two-line fix aligns --jsx-* CLI flag handling with Pragma::default(), bunfig, and JSBundler (all default development: true), and NODE_ENV=production still overrides via set_production().
What was reviewed:
- Verified
development: truematchesPragma::default()(options_types/jsx.rs:201), bunfig'sjsx_dev = truedefault (bunfig.rs:953), andJSBundler.rs:191. - Confirmed the bunfig-merge test cases added in c9f4327 exercise line 1467 — bunfig.rs:990-1013 unconditionally populates
ctx.args.jsxfor any bunfig, and thejsx = "react-jsx"/"react-jsxDEV"cases map correctly tojsx_dev = false/true. - Checked
bun buildis unaffected: without--jsx-*flags it already usedPragma::default()(dev), and--productionstill forcesjsx.development = falseat build_command.rs:461-464.
Extended reasoning...
Overview
Two-line change in src/runtime/cli/Arguments.rs: when any --jsx-* CLI flag is passed and an api::Jsx struct is constructed, the development field now defaults to true (fresh-construct branch, line 1451) or preserves prev.development from bunfig (merge branch, line 1467), instead of hardcoding false in both. A new 17-case parameterized test in test/bundler/transpiler/jsx-cli-flags.test.ts covers the flag × NODE_ENV × bunfig matrix.
Security risks
None. This is JSX transpilation dev/prod-mode selection — no auth, crypto, filesystem, or network surface involved.
Level of scrutiny
Low-to-medium. The source change is 2 lines and mechanically aligns a default with three existing sources of truth (Pragma::default() at options_types/jsx.rs:201, bunfig.rs:953, JSBundler.rs:191). The behavior change is user-visible but strictly a bugfix: previously, adding an unrelated --jsx-* flag silently flipped dev→prod, which was inconsistent with the no-flag path. NODE_ENV=production continues to override via set_production() in transpiler.rs, so the production path is unaffected. I also confirmed bun build behavior is consistent — it already defaulted to dev JSX without --jsx-* flags (via Pragma::default()), and --production still forces development: false explicitly at build_command.rs:461-464.
Other factors
All three of my earlier review threads are resolved: the test-file-location nit was addressed by updating the PR description with a stated reason for the separate file; the missing bunfig-merge-branch coverage was fixed in c9f4327 (author verified reverting line 1467 alone now fails 3 cases); the pre-existing --jsx-side-effects gate omission was acknowledged and reasonably deferred to a follow-up since it needs a different test shape (bun build output assertion vs. runtime import). CodeRabbit's suggestion to expand the matrix was declined with sound reasoning (all flags share one gate, no per-flag branching) and withdrawn. The test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, exit-code asserted last, describe.concurrent), and bunEnv already deletes NODE_ENV so the undefined cases are hermetic.
There was a problem hiding this comment.
LGTM — the bunfig-merge coverage gap I flagged is now closed (17-case matrix exercises both the fresh-construct and prev.development branches, and both bunfig react-jsx/react-jsxDEV directions), and the separate-file rationale is documented. Two-line fix aligns with Pragma::default() (development: true at src/options_types/jsx.rs:201); NODE_ENV=production cases in the matrix confirm the prod path is unaffected.
Extended reasoning...
Overview
Two-line change in src/runtime/cli/Arguments.rs: when any --jsx-* CLI flag triggers construction of api::Jsx, default development to true (fresh-construct branch, matching Pragma::default() at src/options_types/jsx.rs:201) and preserve prev.development (bunfig-merge branch) instead of hardcoding false. New test file test/bundler/transpiler/jsx-cli-flags.test.ts with a 17-case concurrent matrix over --jsx-* flags × NODE_ENV × bunfig presence/content, asserting which of jsx-runtime/jsx-dev-runtime a shim react package reports.
Security risks
None. This is CLI option-plumbing that only affects which JSX runtime import specifier the transpiler emits; no untrusted-input parsing, auth, crypto, or filesystem semantics change.
Level of scrutiny
Low-to-medium. The source change is two field initializers whose new values are demonstrably the defaults already used everywhere else (Pragma::default(), bunfig, JSBundler). The test proves both that the regression is fixed and that NODE_ENV=production still selects the production runtime on both branches, so no behavior is lost. I verified bunfig.rs:961-966 accepts the exact "react-jsx"/"react-jsxDEV" spellings the test uses and sets jsx_dev accordingly, so the merge-branch cases exercise prev.development in both true and false states.
Other factors
All prior review threads are resolved: the comment-cop note was addressed (comment dropped), my test-organization nit was answered with a concrete debug+ASAN timeout justification now recorded in the PR description, my coverage-gap finding was fixed in c9f4327 (author confirmed reverting either changed line independently now fails tests), and CodeRabbit withdrew its matrix-expansion suggestion. The pre-existing --jsx-side-effects gate omission I noted was explicitly and reasonably deferred as out-of-scope (different test shape, tracked separately). The test follows harness conventions: tempDir, bunEnv spread with NODE_ENV explicitly deleted when unset, concurrent pipe drain, stderr/stdout asserted before exit code, describe.concurrent for independent subprocess spawns.
|
CI status: the diff is green. Remaining red in #84493 is unrelated to this change:
Ready for review. |
What
Passing any
--jsx-*CLI flag (--jsx-import-source,--jsx-fragment,--jsx-factory,--jsx-runtime) tobun <file.jsx>silently flipped the automatic JSX runtime from development (jsxDEV/<source>/jsx-dev-runtime) to production (jsx/<source>/jsx-runtime), even when the flag had nothing to do with dev/prod selection. SettingNODE_ENV=developmentin the environment did not restore it; only an explicit--define process.env.NODE_ENV='"development"'did.Repro
Cause
src/runtime/cli/Arguments.rsconstructs anapi::Jsxwhen any--jsx-*flag is present and hardcodeddevelopment: falsein both the fresh-construct branch and the merge-with-bunfig branch. With no--jsx-*flags,transform.jsxstaysNoneandBundleOptions::from_apikeepsPragma::default(), which hasdevelopment: true. So the presence of any flag re-derived JSX options without the development default, and nothing downstream on thebun runpath restores it unlessNODE_ENVreaches the define map (theLoadAllWithoutInliningenv behavior used bybun runskips that injection).Fix
Default
development: trueto matchPragma::default(), bunfig, andJSBundler. In the bunfig-merge branch, preserveprev.developmentinstead of overwriting it.NODE_ENV=productionstill switches to the production runtime viaset_production()exactly as before.Verification
New parameterized test in
test/bundler/transpiler/jsx-cli-flags.test.tsspawnsbun a.jsxwith a shimreactpackage whosejsx-runtimeandjsx-dev-runtimereport which one was imported, across the matrix of--jsx-*flags andNODE_ENVvalues. 6 of the 12 cases fail before this change (all--jsx-*+ dev cases emitPROD); all 12 pass after.The test lives in its own file rather than
jsx-production.test.tsbecause the existing describe block in that file spawns a heavy fixture (react-dom render +Bun.build+ dynamic import) that races the 5s default timeout under debug+ASAN builds, independently of this change.[review] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 3
evidence per changed file