Repository navigation
Conversation
FileMap::result_for_key built the synthetic resolver Result with a
default jsx pragma. The entry point path (enqueue_entry_item ->
ParseTask::init) parses with whatever the Result carries, the same way
it picks up the tsconfig-merged pragma for on-disk files, so an
in-memory entry point was always transpiled with the default automatic
react runtime regardless of Bun.build({ jsx }). Imports of in-memory
files were unaffected because both import paths overwrite the task's
jsx from the transpiler options afterwards.
FileMap::resolve now takes the transpiler's pragma and stamps it into
the Result it fabricates, so every consumer of a FileMap result sees the
configured pragma.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: reproduced and fixed.
|
There was a problem hiding this comment.
LGTM — targeted fix that seeds Result.jsx for in-memory files the same way the disk resolver does.
What was reviewed:
- Traced all three
FileMap::resolvecallers (enqueue_entry_points_normal,run_resolver,resolve_import_records) — each now passes its target transpiler'soptions.jsx, andPragmaisClone. - Checked the new
unsafe { &(*transpiler).options.jsx }inrun_resolver— it is a shared read covered by the existing SAFETY note on thetranspilerraw pointer, and the borrow ends inside the call (pragma is cloned). - Confirmed
enqueue_entry_itemonly patches.developmentafterParseTask::init, so factory/fragment/runtime/import_source now flow through unchanged for in-memory entry points, matching disk entry points. - Tests cover classic factory/fragment, automatic
importSourcewith bothdevelopmentstates, and the plugin-declinedrun_resolverpath; the strengthened existing test would have caught the original bug.
Extended reasoning...
Overview
The PR fixes a bug where Bun.build({ files: {...}, jsx: {...} }) ignored the jsx option for entry points that come from the in-memory files map. FileMap::result_for_key fabricated a bun_resolver::Result with ..Default::default(), so Result.jsx was Pragma::default(), and ParseTask::init parsed with that default. Imports of in-memory files were unaffected because their code paths overwrite task.jsx from transpiler options after ParseTask::init.
The fix adds a jsx: &Pragma parameter to FileMap::resolve/result_for_key and clones it into the fabricated Result. The three callers (entry-point enqueue, run_resolver for plugin-declined imports, and resolve_import_records) each pass their target transpiler's options.jsx. This mirrors what the real resolver does for on-disk files (seeding Result.jsx from resolver options at src/resolver/resolver.rs:1200), so the fix is placed at the layer that owns the invariant rather than adding another consumer-side override.
Security risks
None. This threads an already-computed configuration value (transpiler.options.jsx) into a struct that previously received a default. No untrusted input parsing, no new I/O, no new unsafe primitives — the one added unsafe block is a shared deref of an existing raw pointer under an existing SAFETY justification, used only to read and clone a config struct.
Level of scrutiny
Low-to-medium. The source change is ~10 effective lines: a new parameter threaded to three call sites plus a .clone() into the fabricated Result. Pragma derives Clone and is documented as being cloned per-resolve already, so this adds no new ownership concerns. I verified in enqueue_entry_item (bundle_v2.rs:2775-2790) that the only post-ParseTask::init mutation is .development, which now applies on top of the configured pragma exactly as it does for disk entry points. The two import-path callers already overwrote task.jsx afterwards, so for them the new value in Result.jsx is redundant but harmless (same value assigned twice), keeping the change minimal and behavior-preserving on those paths.
Other factors
Test coverage is thorough for the size of the fix: the existing "in-memory file with JSX" test is strengthened from toContain("Hello JSX") to asserting the exact classic-runtime output and not.toContain("jsxDEV"); three new tests pin classic factory/fragment on both entry and child, automatic importSource with development true and false, and the run_resolver (onResolve-declined) path with an assertion that the plugin actually ran. The PR description states these fail on the released build and pass with the fix, and that bundler_jsx.test.ts, bun-build-api.test.ts, and html-import-manifest.test.ts remain green. No prior reviewer comments to address; the automated bug-hunt found nothing.
There was a problem hiding this comment.
LGTM — targeted fix that seeds the synthetic bun_resolver::Result with the transpiler's JSX pragma, mirroring what the on-disk resolver already does.
What was reviewed
- Confirmed all three
FileMap::resolvecallers and all threeresult_for_keysites are updated; no other callers exist. - Checked the
run_resolverunsafe shared read of(*transpiler).options.jsx— it falls under the existing SAFETY note directly above and does not alias any live&mut. - The strengthened "in-memory file with JSX" test was previously vacuous (only checked for the literal text); it now asserts the exact factory call and rules out
jsxDEV. New tests cover factory/fragment, importSource + development, and the plugin-declinedrun_resolverpath.
Extended reasoning...
Overview
The PR threads a jsx: &Pragma parameter through FileMap::resolve → result_for_key in src/bundler/bundle_v2.rs and stores jsx.clone() on the fabricated bun_resolver::Result. The three call sites (enqueue_entry_points_normal, resolve_import_records, run_resolver) each pass their target's transpiler.options.jsx. Four tests are added/strengthened in test/bundler/bundler_files.test.ts.
Security risks
None. This is pure transpiler-config plumbing for the Bun.build files: option; no untrusted input parsing, no auth, no FFI surface change. The one unsafe block is a shared read through a raw *mut Transpiler that the surrounding function already launders for borrowck reasons, with a SAFETY comment pointing at the existing justification a few lines above.
Level of scrutiny
Low-to-medium. The change is small (~20 net lines of Rust), mechanical, and placed at the layer that owns the invariant: the real resolver seeds Result.jsx from its options for on-disk files, and this makes the in-memory path do the same rather than adding another consumer-side override. The two import paths already overwrote the pragma after ParseTask::init, so their behavior is unchanged (the new value now matches what they overwrite it with); only the entry-point path changes behavior, from Pragma::default() to the configured pragma.
Other factors
- Grep confirms exactly three
file_map.resolve(callers and threeresult_for_key(sites, all updated in the diff. Pragma: Cloneis already relied on at bundle_v2.rs:6234, sojsx.clone()is not new surface.- Tests assert exact transpiled output (
myFactory("div", ...), import source string) and negative conditions (not.toContain("jsxDEV")), covering classic and automatic runtimes plus dev/prod. The plugin-declined test guardsresolveCalledso it can't vacuously pass. Author reports the new tests fail underUSE_SYSTEM_BUN=1and pass on the debug build; adjacent suites (bundler_jsx, bun-build-api, html-import-manifest) also pass. - The comment-cop bot flagged verbose doc comments on earlier commits; those were removed in 957e723 and both threads are resolved.
- No CODEOWNERS entry covers
src/bundler/ortest/bundler/.
Problem
Bun.build({ entrypoints: ["/v/entry.jsx"], files: { "/v/entry.jsx": ... }, jsx: { runtime: "classic", factory: "myH" } })ignores thejsxoption for the entry point: the file is transpiled with the default automatic React runtime, so the build either fails withResolveMessage: Could not resolve: "react/jsx-dev-runtime"or, when react happens to be installed, silently bundles react.jsx.importSourceandjsx.developmentare dropped the same way.FileMap::result_for_key(src/bundler/bundle_v2.rs:1039) fabricates thebun_resolver::Resultfor an in-memory file with..Default::default(), soResult.jsxisjsx::Pragma::default().enqueue_entry_points_normal(bundle_v2.rs:3078) hands that result toenqueue_entry_item, andParseTask::init(src/bundler/ParseTask.rs:261) parses withresolve_result.jsx;enqueue_entry_itemonly patchesjsx.developmentafterwards.ParseTask::init(enqueue_parse_task, bundle_v2.rs:3622, and the FileMap branch ofresolve_import_records, bundle_v2.rs:6239).Fix
FileMap::resolvetakes the pragma of the transpiler the file is bundled with andresult_for_keystores it in theResultit builds. The three callers passtranspiler.options.jsxfor their target.Result.jsxfrom its options (src/resolver/resolver.rs:1200) before merging the nearest tsconfig. The consumers (enqueue_entry_item,ParseTask::init) are correct to trustResult.jsx, since for disk files it holds the per-file tsconfig-merged value, so the fix belongs where the in-memory result is built rather than in another consumer-side override. Thejsx.developmenthandling inenqueue_entry_itemis unchanged and now applies on top of the configured pragma, exactly as it does for disk entry points. ForBun.buildthe transpiler options are the merged result of thejsxoption, the root tsconfig andNODE_ENV, which is the value the import paths already used, so entry points and imports now agree.importSourcewithdevelopmenttrue and false) fail on the released build and pass with the fix; a fourth new test pins the plugin-declined import path (run_resolver), which is the third caller ofFileMap::resolve.Background
files:(theFileMap) letsBun.buildbundle sources that are not on disk. When a specifier matches a key of the map, the bundler skips the resolver and fabricates thebun_resolver::Resultitself inFileMap::resolve; the file's contents are read from the map later, inParseTask.bun_resolver::Resultis the resolver's per-file answer. Besides the path it carries per-file transpile settings, among them the JSX pragma (jsx: runtime, factory, fragment, import source, development flag), because the nearest tsconfig.json can change them per directory.ParseTask::initcopies these settings into the parse task, and the parser uses the task's copy only.files:goes throughenqueue_entry_item, the same function disk entry points use, while an import of afiles:entry goes throughresolve_import_records(orrun_resolverwhen an onResolve plugin declined it), which have their own code for virtual files. That is why only the entry point was affected.