Repository navigation
Conversation
WalkthroughChangesJSX precedence and tsconfig overrides
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:55 PM PT - Aug 20th, 2026
❌ @robobun, your commit 2b31534 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36728That installs a local version of the PR into your bun-36728 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
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}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bundler/transpiler.rs:697-702— Replacing the wholesaleoptions.jsx = tsconfig.jsx.clone()withmerge_tsconfig_jsx_intodropspackage_name(andclassic_import_source), whichmerge_jsxdoes not copy. With no bunfig/CLI jsx andtsconfig.json: {jsx: "react-jsx", jsxImportSource: "preact"},<div {...spread} key="k" />now emitsimport { createElement } from "react"instead of"preact", breaking preact-only builds. Fix: inmerge_jsx, whenapply.contains(JsxField::ImportSource), also assignout.package_nameandout.classic_import_sourcefromself.jsx.Extended reasoning...
What changed
configure_linker_with_auto_jsxpreviously did a full clone of the root tsconfig's JSX pragma when the user hadn't supplied any explicit JSX config:// old (transpiler.rs:655-659 — jsx_pragma_from_resolver is src.clone()) if self.options.transform_options.jsx.is_none() { self.options.jsx = jsx_pragma_from_resolver(&tsconfig.jsx); }
This PR replaces that with an unconditional field-selective merge:
// new (transpiler.rs:702) merge_tsconfig_jsx_into(tsconfig, &mut self.options.jsx); // == *out = tsconfig.merge_jsx(mem::take(out))
TSConfigJSON::merge_jsx(tsconfig_json.rs:221-244) only copies the fiveJsxField-tracked members:factory,fragment,import_source,runtime,development. It does not copypackage_nameorclassic_import_source, which the previous full clone carried.Why
package_namemattersTSConfigJSON::parsepopulates it fromjsxImportSource:// tsconfig_json.rs, jsxImportSource arm result.jsx.package_name = str.to_vec().into(); result.jsx.set_import_source(); result.jsx_flags.insert(JsxField::ImportSource);
The parser reads
package_name(notimport_source) to emit the automatic-runtime key-after-spread fallback import — thecreateElementcall the automatic runtime falls back to when akeyprop appears after a spread:// parse_entry.rs:1946, 1965-1972 let package_name: &'a [u8] = p.arena.alloc_slice_copy(&p.options.jsx.package_name); ... let source_import_names = jsx_imports.source_import_names(); // ["createElement"] when jsx_imports.create_element.is_some() if !source_import_names.is_empty() { p.generate_import_stmt(package_name, source_import_names, ...); }
classic_import_sourceis likewise read at parse_entry.rs:491-503 for the require-record path.Step-by-step regression
Setup — CLI
bun build ./index.tsx, no bunfig.toml, no--jsx-*flags:// tsconfig.json { "compilerOptions": { "jsx": "react-jsx", "jsxImportSource": "preact" } }
// index.tsx export const A = (p: any) => <div {...p} key="k" />;
Arguments::parsesetsopts.jsx = None(no--jsx-*flag passed), sooptions.jsxstarts asPragma::default()withpackage_name = Cow::Borrowed(b"react")andset_fields = empty.configure_linker_with_auto_jsxreads the root tsconfig, whosejsx.package_name = "preact"andjsx_flags = {Runtime, Development, ImportSource}.- Before this PR: wholesale clone →
options.jsx.package_name = "preact".
After this PR:merge_jsxcopiesimport_source = ImportSource{dev: "preact/jsx-dev-runtime", prod: "preact/jsx-runtime"},runtime = Automatic,development = false— but leavespackage_name = "react". sync_resolver_opts()clonesoptions.jsx(with the stalepackage_name) intoresolver.opts.jsx; everyresult.jsxin the resolver is seeded fromself.opts.jsx.clone(), and the per-filemerge_jsxat resolver.rs:1644 also does not touchpackage_name. This flows intoParseTask.jsx(bundle_v2.rs:3469 for entry points, :6417 for resolved files).- Parser hits
<div {...p} key="k" />, emits the "key prop after a {...spread}" warning, setsjsx_imports.create_element, and at parse_entry.rs:1968 emits:import { createElement } from "react"; // ← should be "preact"
Impact
For a preact-only project this is a hard
Could not resolve "react"build failure. If bothreactandpreactare installed, it silently imports React'screateElementfor those elements while the rest of the tree uses preact'sjsx— a subtle runtime mismatch. This is a concrete regression introduced by this PR: previously the wholesale clone seededpackage_namecorrectly, and the pre-existingmerge_jsxat resolver.rs:1644 was fine because its input already carried the right value.Fix
In
TSConfigJSON::merge_jsx, whenapply.contains(JsxField::ImportSource), also copy the derived fields — mirroring whatPragma::from_apiandTSConfigJSON::parsedo for the same input:if apply.contains(JsxField::ImportSource) { out.import_source = self.jsx.import_source.clone(); out.package_name = self.jsx.package_name.clone(); out.classic_import_source = self.jsx.classic_import_source.clone(); }
A test in
15844.test.tscoveringtsconfig {jsx: "react-jsx", jsxImportSource: "preact"}with no bunfig jsx keys and a key-after-spread element (asserting the output containsfrom "preact"and notfrom "react") would lock this in.
|
Both review findings addressed in 50dc85b:
|
There was a problem hiding this comment.
Both prior inline findings are addressed in 50dc85b; no new issues found. Deferring to a human because this is a cross-cutting precedence change (bunfig / CLI / Bun.build vs tsconfig) with an acknowledged output-shape change and overlap with #34980 / #36209 / #36269 that needs a maintainer call on which approach lands.
What was reviewed:
merge_jsxnow gated onset_fields.differenceand copiespackage_name/classic_import_sourcealongsideimport_source— the key-after-spreadcreateElementfallback regression is fixed and covered by a test.Arguments.rselse-branch now preservesprev.development/prev.side_effectswhen merging bunfig +--jsx-*; covered by the newreact-jsxDEVtest.resolver.rs--tsconfig-overridefd fix matches theextends-chain path (FD::INVALID);tsconfig-override.test.tsguards removed so stderr must be empty.- Checked the ruled-out concern that
Bun.serveHTML bundler's explicitjsx.developmentcould now be overridden by tsconfig —set_fieldsis populated inJSBundler.rswhendevelopmentis passed, somerge_jsxskips it.
Extended reasoning...
Overview
The PR threads a per-field JsxFieldSet (set_fields) through api::Jsx → Pragma so that TSConfigJSON::merge_jsx can compute jsx_flags.difference(set_fields) and only fill fields the user did not explicitly set via bunfig / --jsx-* / Bun.build({jsx}). It also replaces the wholesale options.jsx = tsconfig.jsx copy in configure_linker_with_auto_jsx with the same merge, and fixes the --tsconfig-override "directory mismatch" fd warning by passing FD::INVALID instead of the root DirInfo's cached fd. Touches options_types/{jsx,schema}.rs, bunfig.rs, Arguments.rs, JSBundler.rs, js_bundle_completion_task.rs, resolver.rs, tsconfig_json.rs, transpiler.rs, plus docs and three test files.
Security risks
None identified. This is config-precedence plumbing for the JSX transform; no auth, crypto, network, or untrusted-input parsing changes. The FD::INVALID change routes to an absolute-path open instead of openat(wrong_fd, basename), which is strictly safer.
Level of scrutiny
Medium-high. The mechanical parts (adding JsxFieldSet, populating it at each entry point, gating merge_jsx) are straightforward and well-tested. But this is a user-visible behavior change — Bun.build({jsx: {runtime: "automatic"}}) with a "jsx": "react-jsx" tsconfig now emits production jsx instead of jsxDEV (one existing snapshot updated to reflect this), and the PR description explicitly flags overlap with three other open PRs (#34980 identical fd fix, #36269 different mechanism for the same bundler-side precedence problem, #36209 orthogonal development default change). A maintainer should decide which approach lands and how the others rebase.
Other factors
Two prior review rounds from me found (1) the Arguments.rs else-branch dropping prev.development and (2) merge_jsx not copying package_name/classic_import_source — both fixed in 50dc85b with dedicated tests. The regression test file (8 concurrent subprocess tests) covers bunfig-wins, CLI-wins, tsconfig-still-applies-when-bunfig-absent, the package_name key-after-spread fallback, the react-jsxDEV-survives-unrelated-flag case, and the fd-warning fix. tsconfig-override.test.ts had its "ignore the directory-mismatch warning" guards removed so the fix is load-bearing there too. The tsconfig extends chain still merges child-over-parent because base configs' Pragma.set_fields is empty. Given the cross-PR reconciliation and the intentional output change, this warrants human review rather than auto-approval.
There was a problem hiding this comment.
Both earlier findings (the development: prev.development fallthrough and the dropped package_name/classic_import_source in merge_jsx) are fixed with covering tests, and I found nothing new this pass. Deferring to a human for the precedence-semantics decision and the reconciliation with #36209 / #36269 / #34980.
Checked: set_fields propagation through bunfig → Arguments.rs → Pragma::from_api → merge_jsx; the tsconfig extends chain still merges child-over-parent (base's set_fields is empty); configure_linker_with_auto_jsx at both call sites (698, 753) now goes through merge_jsx; the bundler_jsx.test.ts snapshot change matches the described Bun.build({jsx})-now-merges-tsconfig behavior change.
Extended reasoning...
Overview
Adds per-field JsxFieldSet tracking on api::Jsx/Pragma so bunfig / --jsx-* / Bun.build({jsx}) win over tsconfig.json field-by-field, and switches configure_linker_with_auto_jsx from a wholesale tsconfig.jsx clone to merge_jsx. Also fixes the --tsconfig-override "directory mismatch" warning by passing FD::INVALID when the override path lives outside the cached dir fd. 13 files, ~250 net lines.
Security risks
None. Config-precedence plumbing and an fd selection fix; no untrusted-input parsing changes, no new syscall paths.
Level of scrutiny
Moderate–high. The mechanism is straightforward, but it changes user-facing precedence semantics for JSX config across three entry points and introduces a cross-cutting set_fields bitset that must stay consistent across bunfig, CLI, JSBundler, js_bundle_completion_task, Pragma::from_api, and merge_jsx. The Bun.build({jsx}) behavior change (previously skipped tsconfig JSX entirely, now merges) is acknowledged in the description and reflected in one updated snapshot; a maintainer should confirm that's the intended contract.
Other factors
My two prior inline findings on this PR were both addressed in 50dc85b with dedicated tests (bunfig jsx = 'react-jsxDEV' survives an unrelated --jsx-* flag and tsconfig jsxImportSource reaches the key-after-spread createElement fallback). The new 15844.test.ts covers the main precedence matrix and the fd-warning fix; tsconfig-override.test.ts had its warning-masking guards removed so stderr must now be empty. The PR self-reports overlap with three open PRs (#36209, #36269, #34980) that touch the same precedence machinery via different mechanisms — that reconciliation is a maintainer call, not something I should approve around.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/transpiler.rs (1)
692-703: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve
NODE_ENVprecedence over tsconfig JSX.
configure_env_for_runcallsconfigure_linker()beforerun_env_loader(). WithNODE_ENV=productionand"jsx": "react-jsxDEV", the later tsconfig merge restoresjsx.development = true. This path does not callconfigure_defines()to restore the production setting. Apply the tsconfig merge before the environment override, or skipJsxField::Developmentwhen production is forced. Add a regression test.🤖 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/transpiler.rs` around lines 692 - 703, Update the auto-JSX configuration flow around merge_tsconfig_jsx_into so a production NODE_ENV override remains authoritative over tsconfig’s JSX development setting; either perform the tsconfig merge before the environment override or exclude JsxField::Development when production is forced. Add a regression test covering NODE_ENV=production with jsx set to react-jsxDEV and verify development mode remains disabled.
🤖 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.
Outside diff comments:
In `@src/bundler/transpiler.rs`:
- Around line 692-703: Update the auto-JSX configuration flow around
merge_tsconfig_jsx_into so a production NODE_ENV override remains authoritative
over tsconfig’s JSX development setting; either perform the tsconfig merge
before the environment override or exclude JsxField::Development when production
is forced. Add a regression test covering NODE_ENV=production with jsx set to
react-jsxDEV and verify development mode remains disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 583c1861-4a02-47a1-af6f-aa2179ee4e08
📒 Files selected for processing (13)
docs/runtime/bunfig.mdxsrc/bundler/transpiler.rssrc/bunfig/bunfig.rssrc/options_types/jsx.rssrc/options_types/schema.rssrc/resolver/resolver.rssrc/resolver/tsconfig_json.rssrc/runtime/api/JSBundler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/cli/Arguments.rstest/bundler/bundler_jsx.test.tstest/cli/run/tsconfig-override.test.tstest/regression/issue/15844.test.ts
|
Re the CodeRabbit # tsconfig.json: {"compilerOptions": {"jsx": "react-jsxdev"}}
NODE_ENV=production bun ./a.jsx # -> PROD
NODE_ENV=production bun run ./a.jsx # -> PROD
bun ./a.jsx # -> DEV
|
There was a problem hiding this comment.
Both earlier findings (the development: prev.development fallthrough and merge_jsx dropping package_name/classic_import_source) are fixed in 50dc85b and covered by new tests; I found nothing further this pass. Deferring to a human because this is a user-visible precedence change (one existing bundler_jsx snapshot flipped from jsxDEV to jsx) and it overlaps with #36209 / #36269 / #34980, so a maintainer should pick which approach lands.
What was reviewed:
merge_jsxnow gates onjsx_flags.difference(set_fields)— checked that the tsconfigextends-chain merge (base has emptyset_fields) and the per-resolveresolver.rs:1644call still layer child-over-parent correctly.resolver.rsfd fix usesFD::INVALIDonly whentsconfig_override.is_some(); the discovered-tsconfig path keeps the cached dir fd.bunfig.rsnow leavesctx.args.jsx = Nonewhen nojsx*keys are present, so tsconfig still applies (covered by the "bunfig has no jsx keys" test).- Confirmed
jsx_pragma_from_resolveris still live (transpiler.rs:2850), not newly-dead.
Extended reasoning...
Overview
Introduces per-field JsxField/JsxFieldSet tracking on api::Jsx and Pragma so that bunfig / --jsx-* / Bun.build({jsx}) values survive the tsconfig merge instead of being clobbered. merge_jsx now computes jsx_flags.difference(current.set_fields) and only applies the remainder; configure_linker_with_auto_jsx switches from a wholesale tsconfig.jsx clone to that merge. The --tsconfig-override fd-mismatch warning is fixed by passing FD::INVALID so parse_tsconfig opens by absolute path. Touches options_types/{jsx,schema}.rs, bunfig.rs, Arguments.rs, JSBundler.rs, js_bundle_completion_task.rs, resolver.rs, tsconfig_json.rs, transpiler.rs, plus docs and three test files.
Security risks
None. This is config-precedence plumbing; no untrusted input parsing, auth, or crypto is touched. The resolver change narrows an fd usage rather than widening it.
Level of scrutiny
Medium-high. The mechanism is straightforward (a bitset gating field copies), but it threads through four config entry points and changes user-visible output — the jsx/sideEffectsTrueTsconfigAutomatic snapshot now emits jsx() instead of jsxDEV() because tsconfig's development: false fills a gap the user didn't set. Two real bugs were caught in earlier review passes on this PR (both fixed and tested), which is evidence the merge matrix is easy to get subtly wrong.
Other factors
- Overlapping PRs: the description names #36209, #36269, and #34980 as touching the same precedence / fd-warning ground with different mechanisms. A maintainer should decide which lands and how to reconcile.
- Test coverage: 8 new concurrent subprocess tests in
15844.test.tscover bunfig-runtime, bunfig-importSource, bunfig-factory, CLI--jsx-runtime, bunfig-with-no-jsx-keys, key-after-spreadpackage_name, bunfigreact-jsxDEV+ unrelated CLI flag, and the fd warning. Thetsconfig-override.test.tsguards that used to filter out the "directory mismatch" warning are removed so stderr must be empty. - Behavior change: acknowledged in the PR description and reflected in one updated snapshot; whether that's the desired precedence for
Bun.build({jsx})+ tsconfig is a product call.
|
Status: diff is ready at 2b31534, squashed to one commit and rebased onto current main (past #39631 and #39732; conflict resolution is in the Rebase notes section of the description).
|
robobun
changed the title
|
Consolidation note: the |
b97aa8c to
3845a4b
Compare
There was a problem hiding this comment.
Both earlier nits are addressed (stderr now asserts the key-after-spread warning; title/description no longer reference #22023), and this pass found no further issues. Leaving the merge call to a maintainer given the documented Bun.build({jsx}) × tsconfig behavior change and the pending reconciliation with #36269's competing approach.
What was reviewed:
merge_jsx's newjsx_flags.difference(set_fields)gate — confirmed the tsconfigextendschain still merges child-over-parent (base'sset_fieldsis empty).transpiler.rsswitching from wholesale clone tomerge_tsconfig_jsx_into—side_effects/parseare preserved from the incomingPragma, andpackage_name/classic_import_sourcenow copy alongsideimport_source.Arguments.rselse-branch —prev.development/prev.side_effectsfallthrough is in place; theopts.jsx.is_none()branch'sdevelopment: falseis unchanged pre-existing behavior with no bunfig/CLIDevelopmentbit set, so tsconfig can still fill it.bunfig.rs— an unrecognizedjsx = "…"value leavesjsx_setempty and no longer materializes a defaultapi::Jsx, so tsconfig still applies.
Extended reasoning...
Overview
The PR threads a per-field JsxFieldSet (set_fields) through api::Jsx → Pragma so that TSConfigJSON::merge_jsx can skip fields the user explicitly set via bunfig, --jsx-* flags, or Bun.build({jsx}). It touches the option type definitions (options_types/{jsx,schema}.rs), all three config entry points (bunfig.rs, Arguments.rs, JSBundler.rs), the tsconfig merge (tsconfig_json.rs), the transpiler's root-tsconfig application (transpiler.rs), and the completion-task Pragma construction. Tests cover bunfig/CLI overriding tsconfig, tsconfig still applying when bunfig has no jsx keys, the key-after-spread package_name path, and bunfig react-jsxDEV surviving an unrelated CLI flag. One bundler_jsx.test.ts snapshot updated to reflect the documented behavior change.
Security risks
None. This is config-precedence plumbing for JSX transpile options; no untrusted input parsing, no filesystem/network surface, no auth or crypto.
Level of scrutiny
Moderate-to-high. The mechanical changes are straightforward, but the PR (a) changes user-visible Bun.build({jsx}) × tsconfig semantics (previously specifying jsx skipped tsconfig entirely; now unset fields fall through), (b) introduces a design choice — per-field set_fields tracking — that the PR description itself notes conflicts with #36269's different mechanism, and (c) touches config precedence across four entry points where subtle ordering matters. That is a maintainer-level design/compat decision, not a mechanical fix.
Other factors
All four of my earlier findings across two review passes were addressed (the two 🔴 bugs in 50dc85b, the two 🟡 nits by 3845a4b and the metadata edit). Buildkite #87454 was green on all relevant lanes per the author's status comment; a fresh build (#94016) is running on the latest commit. The evidence gate confirmed fail-before/pass-after on both ASAN and release. The remaining reasons not to auto-approve are already stated by the author in the PR description, so this defer is just confirming the automated review is clean on the latest revision.
3845a4b to
3477cdf
Compare
There was a problem hiding this comment.
I reviewed this PR across five passes and the current head (66dac82) addresses everything raised; this run found no further issues. Because it changes config-precedence semantics across bunfig / CLI / Bun.build({jsx}) / tsconfig, carries an acknowledged behavior change to Bun.build({jsx}) + tsconfig, and the description notes #36269 solves an overlapping problem with a different mechanism that will need reconciling, a human look at the precedence design is still worthwhile.
What was reviewed:
merge_jsxnow gated onset_fields.difference; verified the tsconfigextendschain still merges child-over-parent (base'sset_fieldsis empty).configure_linker_with_auto_jsxno longer dropspackage_name/classic_import_source— covered by the key-after-spread test.runner.rswas the only internal--jsx-*emitter; forwarding is nowset_fields-gated and the serial/parallel parity test guards it.- The
Arguments.rsbunfig+CLI merge branch now falls back toprev.development/prev.side_effects; covered by thereact-jsxDEVtest.
Extended reasoning...
Overview
The PR threads a per-field JsxFieldSet (set_fields) through every JSX config producer — bunfig.toml, --jsx-* CLI flags, Bun.build({jsx}) — and into Pragma, then makes TSConfigJSON::merge_jsx skip fields already in that set. It also switches configure_linker_with_auto_jsx from a wholesale tsconfig-jsx clone to a merge, updates the bun test --parallel worker-argv builder to only forward user-set jsx fields, moves the JsxField enum from the resolver into bun_options_types, and adds docs + 8 regression tests. 14 files, ~249 net lines.
Security risks
None identified. All inputs are local config (bunfig, tsconfig, CLI flags, Bun.build options); no network, auth, or untrusted-data parsing is touched. The change is purely about which config layer wins for JSX transform selection.
Level of scrutiny
High. This is a cross-cutting semantic change to config precedence, not a mechanical fix: it alters what Bun.build({jsx: {...}}) emits when a tsconfig is also present (previously tsconfig JSX was skipped entirely; now unset fields fall through to it — one existing snapshot in bundler_jsx.test.ts was updated to reflect this). The five prior review passes each found a real issue (dropped package_name, hardcoded development: false in the CLI merge branch, the parallel-runner unconditionally forwarding --jsx-runtime, test-strength gaps), all of which were fixed with dedicated tests. That history plus the description's own note that #36269 addresses the same interaction "with a different mechanism and will need reconciling" means the precedence design itself deserves a human decision before merge.
Other factors
All prior review threads are resolved. Test coverage is thorough (bunfig/CLI/import-source overrides, tsconfig fallthrough, key-after-spread package_name, bunfig-dev + unrelated CLI flag, serial vs --parallel=2 parity), and the gate evidence shows the new tests fail on main and pass here. The enumset removal from src/resolver/Cargo.toml is correct — JsxField moved to bun_options_types and the resolver re-exports it from there. Given the scope, the behavior change, and the open reconciliation question with #36269, deferring to a human rather than auto-approving.
A JSX field set explicitly in bunfig.toml, via a --jsx-* CLI flag, or via
Bun.build({jsx}) is no longer overwritten by a tsconfig.json that sets the
same field. Fields not set explicitly still fall through to tsconfig.
TSConfigJSON::merge_jsx applied every field tsconfig set regardless of
whether the incoming Pragma already carried a user-set value, and the
user-set side had no per-field was-set tracking to gate it.
- options_types::jsx: add JsxField/JsxFieldSet (moved from the resolver)
and Pragma.set_fields; carry it through api::Jsx and Pragma::from_api.
The enumset dependency moves from the resolver to options_types with it.
- bunfig.rs: populate set_fields per jsx* key and stop materialising an
api::Jsx when no jsx* keys are present.
- Arguments.rs / JSBundler.rs: populate set_fields per --jsx-* flag /
Bun.build({jsx}) option. The merge-with-bunfig branch in Arguments.rs
now falls back to prev.development / prev.side_effects like the other
fields instead of hardcoding false.
- tsconfig_json.rs: merge_jsx skips fields already in set_fields, and
copies package_name / classic_import_source along with import_source so
the key-after-spread createElement fallback imports from the configured
package.
- transpiler.rs configure_linker_with_auto_jsx: merge the root tsconfig
(honouring set_fields) instead of a wholesale copy or a skip.
- test/parallel/runner.rs: forward a --jsx-* flag to workers only when
its field is in set_fields. Forwarding the default runtime would mark it
user-set in the worker and block the worker's tsconfig, so a parallel
run would transpile differently from a serial one.
Fixes #15844.
66dac82 to
2b31534
Compare
There was a problem hiding this comment.
I reviewed this PR and the bug hunting system found no issues this pass; every earlier finding has been addressed and resolved. Because it changes user-facing JSX config precedence across four sources (bunfig / --jsx-* / Bun.build({jsx}) / tsconfig), carries an intentional behavior change to Bun.build({jsx}) + tsconfig, and the description flags #36269 as needing reconciling with the set_fields approach, a maintainer sign-off on the precedence model is still worthwhile.
What was reviewed:
merge_jsx's newjsx_flags.difference(set_fields)gating and the tsconfigextendschain (base config'sset_fieldsis empty, so child-over-parent still holds).set_fieldspropagation through every producer: bunfig,Arguments.rs,JSBundler.rs,js_bundle_completion_task.rs, and the--parallelworker argv forwarder.- The
transpiler.rswholesale-copy → merge switch, includingpackage_name/classic_import_sourcenow copied alongsideimport_source(covered by the key-after-spread test). hash_for_runtime_transpiler—set_fieldsis precedence metadata only and correctly not hashed; the fields it gates already are.
Extended reasoning...
Overview
This PR fixes #15844 by giving bunfig.toml jsx* keys, --jsx-* CLI flags, and Bun.build({jsx}) per-field precedence over tsconfig.json's compilerOptions.jsx*. It introduces JsxField/JsxFieldSet on api::Jsx and Pragma, populates the set at each config source, and makes TSConfigJSON::merge_jsx skip fields already in set_fields. It touches 15 files across options_types, bunfig, resolver, bundler, runtime/api, runtime/cli (including the bun test --parallel worker argv builder), docs, and adds an 8-case regression suite plus one updated bundler snapshot.
Security risks
None. This is transpiler-config precedence; no untrusted-input parsing, no auth, no filesystem/network surface changes. The enumset dependency move is a workspace-internal Cargo shuffle.
Level of scrutiny
Moderate-to-high. The mechanism (a bitset gating a merge) is small, but it threads through five producers and two merge sites, and it changes observable output for anyone combining Bun.build({jsx}) or bunfig with a tsconfig. The PR has already been through four review iterations that caught real bugs (dropped package_name, hardcoded development: false in the CLI+bunfig branch, the parallel-runner unconditional --jsx-runtime forward), all of which are now fixed and covered by tests. The remaining question is design-level: the precedence model itself, the intentional Bun.build({jsx}) dev→prod behavior change, and how this reconciles with #36269's competing approach — decisions a maintainer should own.
Other factors
Test coverage is thorough (bunfig runtime/factory/importSource overrides, CLI override, tsconfig-still-applies-when-bunfig-absent, key-after-spread package_name propagation, bunfig react-jsxDEV surviving an unrelated CLI flag, and serial vs --parallel=2 parity). All prior review threads are marked resolved. The updated bundler_jsx.test.ts snapshot reflects the documented behavior change and its sideEffects invariant is unchanged. The enumset removal from src/resolver/Cargo.toml is correct (JsxField moved to bun_options_types and is re-exported).
Fixes #15844.
Repro
Cause
TSConfigJSON::merge_jsxapplied every field tsconfig set, regardless of whether the incomingPragmaalready carried a user-set value. The user-set side (api::Jsx/Pragma) had no per-field was-set tracking, so nothing could gate that overwrite.configure_linker_with_auto_jsxlikewise did a wholesaleoptions.jsx = tsconfig.jsxcopy (or skipped tsconfig entirely) instead of merging.Fix
options_types::jsx: addJsxField/JsxFieldSet(moved from the resolver) andPragma.set_fields; carry it throughapi::JsxandPragma::from_api.bunfig.rs: populateset_fieldsfor eachjsx*key actually present, and stop materialising anapi::Jsxwhen nojsx*keys are present (previously every bunfig load created one with default values, which itself masked tsconfig).Arguments.rs/JSBundler.rs: populateset_fieldsfor each--jsx-*flag /Bun.build({jsx})option passed.tsconfig_json.rs:merge_jsxnow computesjsx_flags.difference(current.set_fields)and only applies those. The tsconfigextendschain still merges child-over-parent because the base config'sset_fieldsis empty.transpiler.rsconfigure_linker_with_auto_jsx: always merge the root tsconfig instead of the wholesale copy, so explicit user config survives and non-tracked fields likeside_effectsare preserved.test/parallel/runner.rs: thebun test --parallelcoordinator re-emits the jsx config to each worker as--jsx-*flags. It forwarded--jsx-runtimewhenever any jsx config existed, which under the new rule would mark the runtime user-set in the worker and block its tsconfig. It now forwards only the fields inset_fields, so a parallel run transpiles the same as a serial one.docs/runtime/bunfig.mdx: document the precedence.Precedence
--jsx-*/bunfig.tomljsx*/Bun.build({jsx})>tsconfig.jsoncompilerOptions.jsx*> defaults, per field. A field not set in the higher tier still falls through to tsconfig.Behavior change
Bun.build({jsx: {runtime: "automatic"}})with atsconfig.jsonthat sets"jsx": "react-jsx"now emits the productionjsxruntime (tsconfig'sdevelopment: falsefills the gap the user did not set) instead ofjsxDEV. PreviouslyBun.build({jsx})caused tsconfig JSX to be skipped entirely. OnejsxSideEffectssnapshot inbundler_jsx.test.tsupdated to reflect this; itssideEffectsassertion is unchanged.Verification
test/regression/issue/15844.test.tscovers bunfigjsx/jsxFactory/jsxImportSourceoverriding tsconfig,--jsx-runtimeoverriding tsconfig, tsconfig still applying when bunfig has no jsx keys, thejsxImportSourcekey-after-spread fallback, a bunfigreact-jsxDEVsurviving an unrelated--jsx-*flag, andbun testversusbun test --parallel=2producing the same runtime on one fixture (the bunfig/CLI override cases fail on the released binary and pass here).bundler_jsx.test.ts,esbuild/tsconfig.test.ts,jsx-tsconfig-react-jsx.test.ts,transpiler-tsconfig-uaf.test.ts, andtest/config/bunfig/all pass.Overlap
This PR originally also carried the one-line
--tsconfig-overridefd fix for #22023; that is the same change as #34980, which is now the PR for it, so the resolver hunk and its tests were dropped from this branch (b97aa8c) and the two no longer touch the same files. #36269 addresses the--jsx-*vs tsconfig dev/prod interaction from the bundler side with a different mechanism and will need reconciling with theset_fieldsapproach here. #36209 changes thedevelopmentdefault inArguments.rsand is orthogonal.Rebase notes
The branch is squashed to one commit and rebased onto main. Two rebases needed manual resolution:
jsx: keep the default factory when a tsconfig or pragma factory has no members) changedPragma::from_apito returnPragmainstead ofResult<Pragma>; this PR addspragma.set_fields = jsx.set_fieldsbefore that return. Resolved by keeping both. An explicit but invalid factory such as--jsx-factory=.still marksFactoryas user-set, so it yields the built-in default rather than the tsconfig value; valid flags and tsconfig-only configs behave as before. ThefactoryMemberListtests from jsx: keep the default factory when a tsconfig or pragma factory has no members #39631 pass on this branch.enum-mapfromsrc/resolver/Cargo.tomlandenumsetfromsrc/options_types/Cargo.toml, while this PR removesenumsetfrom the resolver and starts using it inoptions_types. Resolved so the resolver keeps neither andoptions_typesgetsenumsetback. The netCargo.lockchange is that one dependency edge moving frombun_resolvertobun_options_types, which is whereJsxFieldnow lives.[review] gate passed · iteration 4 · 15 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 9 passed · 0 rejected · iteration 4
evidence per changed file