bundler: bundle a file once per requested loader instead of once per path - #37476
bundler: bundle a file once per requested loader instead of once per path#37476robobun wants to merge 4 commits into
Conversation
PathToSourceIndexMap and the per-file ResolveQueue were keyed on the
resolved path alone. A file imported with two different import
attributes (`with { type: "text" }` in one module, a plain import in
another) was therefore bundled once, with whichever loader belonged to
the import that happened to be resolved first. Which importer got the
wrong value depended on parse order, so the output changed from build to
build. The same collapse made `import self from "./entry.mjs" with
{ type: "text" }` fail with "No matching export" because the request was
folded into the JS module.
Both maps now identify a module by (path, loader the import asked for).
The first registration for a path is stored as before; further loaders
for the same path go in a lazily allocated per-loader side table, so the
common case costs one extra byte per entry. Path-only lookups (entry
point dedup, dual-package secondary paths, dev server) keep returning the
first registration. The dev server keeps one module per path, since its
incremental graph is keyed by path.
HTML url assets that get forced to the file loader are covered by the
same rule: a script importing the same manifest.json as JSON no longer
receives the asset's output path.
WalkthroughThe bundler replaces path-only module indexing with loader-aware indexing. Resolution, queueing, linking, HTML handling, and plugin fallback now preserve distinct modules for different loaders. Regression tests cover static, dynamic, self-import, HTML, and plugin cases. ChangesLoader-aware module identity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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/bundle_v2.rs (1)
4750-4762: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the requested loader after successful
onResolve.This path derives
loaderonly fromresult.path. A plugin that successfully resolves./data.jsonfor an import with{ type: "text" }selectsLoader::Json. The bundler then merges or parses the module as JSON instead of text.Capture the effective requested loader before plugin dispatch and carry it in the resolve request or response. Do not recover it from the graph in this callback. Add a regression test where
onResolvereturns a successful file result for text and JSON imports of the same path.🤖 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/bundle_v2.rs` around lines 4750 - 4762, Preserve the import’s requested loader through successful onResolve handling instead of deriving it from result.path. Capture the effective loader before plugin dispatch, carry it through the resolve request or response, and use that value in the path-to-source lookup near path_to_source_index_map and get_or_put. Add a regression test covering successful file resolutions for text and JSON imports targeting the same path.
🤖 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/bundle_v2.rs`:
- Around line 4750-4762: Preserve the import’s requested loader through
successful onResolve handling instead of deriving it from result.path. Capture
the effective loader before plugin dispatch, carry it through the resolve
request or response, and use that value in the path-to-source lookup near
path_to_source_index_map and get_or_put. Add a regression test covering
successful file resolutions for text and JSON imports targeting the same path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d4c1761-a787-4f6f-b9c3-29c30acad2c8
📒 Files selected for processing (6)
src/bundler/LinkerContext.rssrc/bundler/PathToSourceIndexMap.rssrc/bundler/bundle_v2.rstest/bundler/bundler_html.test.tstest/bundler/bundler_loader.test.tstest/bundler/bundler_plugin.test.ts
When a plugin's onResolve returned a path, the module was keyed and parsed with the loader of that path's extension, so the import attribute was ignored and text/JSON imports of the same plugin-resolved file still collapsed into one module. Capture the record's loader on the MiniImportRecord at dispatch time and use it both when the plugin returns a path and when it declines. This also stops run_resolver from reading the importer's records out of the graph, which may not hold them yet when the callback completes early.
|
Updated 5:57 AM PT - Aug 11th, 2026
✅ @robobun, your commit eb317560a37347ad69093da5fdc3dea8433d7940 passed in 🧪 To try this PR locally: bunx bun-pr 37476That installs a local version of the PR into your bun-37476 --bun |
|
Review round addressed:
PR description updated to match. |
The CJS re-export shortcut and the "use client" proxy both replace an existing map entry. Keying that replacement on the parsed loader sent it to a fresh per-loader slot whenever an onLoad plugin changed the loader, leaving the original registration untouched. Replace the entry that holds the module instead, whichever loader it was registered under.
|
Follow-up to the second review round: the CJS re-export shortcut and the "use client" proxy no longer re-key the map with the post-plugin loader. Both now call |
There was a problem hiding this comment.
eb31756 addresses my earlier note — redirect() finds the entry by value regardless of which loader it was registered under, so the CJS-redirect and "use client" overwrite sites now stay consistent with the registration key. No further issues found this run. Given this reworks module identity across the bundler's graph construction (~15 call sites, the record.loader.is_none() skip in patch_import_record_source_indices, the dev-server one_module_per_path opt-out), a maintainer look would still be worthwhile.
Checked: ModuleMap get/put/get_or_put semantics against every updated call site; that remove/clear/iter cover the side table; that MiniImportRecord.loader is populated at both dispatch sites and consumed at all three onResolve return paths; that the HTML manifest read/write pair both key on Loader::Html. The comment-cop flag on redirect's two-line doc is a false positive.
Extended reasoning...
Overview
The PR changes the bundler to identify a module by (resolved path, requested loader) instead of path alone, so import x from './f.json' and import x from './f.json' with { type: 'text' } become two distinct modules. PathToSourceIndexMap is rewritten as a generic ModuleMap<V> with a primary path-keyed table plus a lazily-allocated per-loader side table; bundle_v2.rs threads the requested loader through ~15 registration/lookup sites (resolve_import_records, process_resolve_queue, run_resolver, the onResolve success/fallthrough/virtual paths, entry-point enqueue, HTML manifest, CJS-redirect, SCB proxy); LinkerContext.rs updates the HTML manifest read side. Seven new itBundled tests cover cross-module, single-module, dynamic-import, self-import, HTML+JSON, and both plugin onResolve paths.
Prior review rounds
My first pass flagged that the onResolve success branch dropped the import attribute — fixed in 5956c95 by adding loader to MiniImportRecord and reading it at all three consumer sites, with a covering test. My second pass noted the two overwrite-style put() calls could miss the registered entry when a plugin changes the loader post-registration — eb31756 replaces both with redirect(path, from_index, to_index), which scans by_path and every side table for the entry holding from_index, so the registration loader no longer needs to be recovered. Both concerns are resolved in the current diff.
Security risks
None. This is pure bundler graph-construction bookkeeping; no untrusted input parsing, filesystem, or network paths are added.
Level of scrutiny
High. Module identity is the invariant every downstream bundler stage (linker, chunking, metafile, dev-server incremental graph) depends on. The change is well-scoped and thoroughly tested, but it touches many parallel call sites where a missed one would reintroduce nondeterminism, and it introduces one behaviour change beyond the map key: patch_import_record_source_indices now skips records with loader == None on the stated basis that only records resolve_import_records queued need patching there. That reasoning reads correctly against resolve_import_records, but it's the kind of invariant a bundler maintainer should confirm.
Other factors
The dev server is deliberately opted out via one_module_per_path because its incremental graph is path-keyed — sensible, and dev-server suites were run per the description, but it means the fix does not apply under HMR. The metafile now emits duplicate inputs keys for a doubly-bundled path (called out as a follow-up). One outstanding comment-cop flag on the two-line redirect doc is a false positive — it's a plain description, not a workaround justification. CI on eb31756 was still building at last update.
bun buildmerged two imports of the same file that used different import attributes into one module. Which loader that module got depended on which importer happened to be parsed first, so the build silently produced different (and wrong) output from run to run. This is the bundler counterpart of #32999, which fixes the same collapse in the runtime module registry and deliberately left the bundler for a separate PR.Reproduction
The bundle contains a single
// j.jsonmodule, and 12 builds of the same input produce two different output hashes. The same collapse makesimport self from "./entry.mjs" with { type: "text" }fail to bundle withNo matching export in "entry.mjs" for import "default"(the text request is folded into the JS module), and makes a script that importsmanifest.jsonas JSON receive the asset's output path when the HTML entry also references that file via<link rel="manifest">(the HTML reference is forced to the file loader). That last one is deterministic, since the HTML entry is always parsed first.Cause
PathToSourceIndexMap(per target) and the per-fileResolveQueueinbundle_v2.rsare keyed on the resolved path text only.resolve_import_recordscomputes the loader each import asked for, but the lookup right after it ignores that loader: the second import of a path reuses whatever task was queued first, and the loader of that first task decides how the file is parsed for everybody.Fix
Both maps now identify a module by (path, loader the import asked for).
PathToSourceIndexMap.rsgets a small genericModuleMap<V>used for both:get/get_pathkeep returning the first registration for callers that identify a module by path alone (entry point dedup, dual-packagesecondary_pathrewrites, dev server invalidation, the dev-only barrel fallbacks).get_with_loader/get_or_put/puttake the loader. The compiler found every registration site; they all already had the loader at hand (import_record_loader, the task loader inprocess_resolve_queue, the extension loader inenqueue_entry_item, the import's attribute loader (captured onMiniImportRecordwhen the plugin request is dispatched) or else the extension loader inrun_resolver/on_resolve,Loader::Htmlfor the HTML import manifest on both the insert side and theLinkerContextread side). The two sites that used to overwrite an entry so later importers land elsewhere (the CJS re-export shortcut and the "use client" proxy) now callredirect(path, from, to), which swaps out whichever entry holds the module being replaced, so they need no loader at all.patch_import_record_source_indiceslooks records up with the loaderresolve_import_recordsstored on them; records that were never queued (internal, external, unresolved, plugin-resolved) get their source index elsewhere and are skipped.one_module_per_path, set instart_from_bake_dev_server): its incremental graph and HMR runtime are keyed by path, so a second module for the same path has nothing to map onto. Its behaviour is unchanged.remove(used byIncrementalGraph::on_file_deleted) also drops side-table entries for the path.onResolveresults used to take the loader from the returned path's extension and drop the import attribute. The attribute now wins there too, as it already did when the plugin declined, so the same file resolved through a plugin is also one module per requested loader.An explicit attribute that names the loader the extension already selects (
import x from "./a.json" with { type: "json" }) still shares the module with a plain import, since the requested loaders are equal.Tests
test/bundler/bundler_loader.test.ts(newdescribe): the cross-module case above, the same two imports inside one module, a static import plusimport(..., { with: { type: "text" } })of the same file, an entry point importing itself as text, and a control thattype: "json"on a.jsonfile is still one module.test/bundler/bundler_html.test.ts: manifest referenced by HTML and imported as JSON by the script.test/bundler/bundler_plugin.test.ts: the cross-module case with a catch-allonResolvethat declines (resolution goes throughrun_resolver), and again with anonResolvethat resolves a virtual specifier to the file on disk (theon_resolvesuccess path).With
USE_SYSTEM_BUN=1the new tests fail (4 in the loader file plus the passing control, the html one, both plugin ones); withbun bdthey pass. The repro above givesstring object 1on every run and one output hash across 12 builds. Also run with the debug build:bundler_html_server,html-import-manifest,bundler_plugin,bundler_barrel,bundler_splitting,bundler_edgecase,esbuild/loader,metafile,esbuild/metafile,bundler_bun,bun-build-api,bundler_files,bundler_plugin_chain,native-plugin, andtest/bake/dev/{bundle,esm,incremental-graph-edge-deletion,production,hot,html,plugins,css}(production needs a longer timeout than the 5s default on a debug build regardless of this change).Notes
inputskey (theimports[].withentries already distinguish the two). esbuild disambiguates such keys with awith { type: ... }suffix; that is a small follow-up inMetafileBuilderand not needed for the fix itself.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_plugin.test.ts