Repository navigation
Conversation
…ines them
Bun.build({ files }) dispatches every entry point matched by an onResolve
filter to the plugin before looking at the files map. When the plugin
returned nothing, on_resolve()'s NoMatch fallback for entry points went
straight to disk resolution, so a virtual entry point failed with
ModuleNotFound even though the same build succeeds without the plugin.
Move the files-map lookup into a helper shared by the no-plugin path and
the NoMatch fallback, so both resolve an entry point the same way: files
map first, then disk.
|
Warning Review limit reached
Next review available in: 4 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: diff is ready; CI needs the two macOS shards re-run. Reproduced on bun 1.4.0 with the snippet in the PR description ( Tests: the new block in CI (build 96269): every lane that ran passed (177 jobs, including the lanes that run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that makes the onResolve NoMatch fallback for entry points consult the files map before disk, matching the no-plugin path and the import path.
Reviewed: the extracted enqueue_entry_point_from_file_map helper is byte-equivalent to the inline block it replaces in enqueue_entry_points_normal, and its new call in on_resolve's NoMatch arm passes the same specifier/target that enqueue_entry_point_on_resolve_plugin_if_needed originally dispatched. Checked that Err(_) => return matches the surrounding let Ok(..) else { return } convention in that callback, and that a map hit correctly skips the entry_point_original_names store (only needed when the specifier and resolved path differ). Tests cover JS/HTML entries, sync/async decline, byte-identical output vs. no-plugin, mixed disk+memory, and the preserved ModuleNotFound error.
Extended reasoning...
Overview
This PR fixes a bug where Bun.build({ entrypoints, files, plugins }) fails with ModuleNotFound for an in-memory entry point when an onResolve plugin's filter matches the entry point but the callback returns undefined (declines). The NoMatch arm in BundleV2::on_resolve for ImportKind::EntryPointBuild went straight to disk resolution (resolve_entry_point) without consulting self.file_map, unlike the no-plugin path in enqueue_entry_points_normal and the import fallback in run_resolver. The fix extracts the file-map lookup into enqueue_entry_point_from_file_map and calls it from both sites, so a declining plugin is a true no-op for entry points as it already is for imports.
Two files touched: ~20 net lines in src/bundler/bundle_v2.rs (helper extraction + one new call site) and ~120 lines of new tests in test/bundler/bundler_files.test.ts.
Security risks
None. This is bundler resolution ordering for the in-process Bun.build API. No new inputs are trusted, no filesystem writes, no auth/crypto/network. The files map is caller-supplied and was already consulted on every other resolution path; this just adds the one path that was missed.
Level of scrutiny
Low-to-moderate. The change is small, mechanical, and well-explained. I traced the data flow: enqueue_entry_point_on_resolve_plugin_if_needed sets specifier = entry_point, namespace = "file", original_target = target; the NoMatch arm reads exactly those back and passes them to the new helper, which does the identical file_map.resolve(arena, b"", entry_point) + enqueue_entry_item(&mut resolved, true, target) that the inline block previously did in enqueue_entry_points_normal. The helper returns Ok(false) when file_map is None or has no hit, so the disk fallback and its error reporting are unchanged. The Err(_) => return on the new call matches the existing let Ok(..) else { return } pattern used for both resolve_entry_point and enqueue_entry_item immediately below it in the same arm.
Other factors
- Test coverage is thorough for a fix of this size: both JS and HTML entry points, sync and async decline, byte-for-byte equality with a plugin-free build (proving the declining plugin is a no-op), a mixed on-disk + in-memory build, and a negative test confirming
ModuleNotFoundstill fires when the entry is in neither the map nor on disk. PR description states 6/7 new tests fail on the released binary and all pass with the debug build. - I confirmed the file-map hit does not need the
entry_point_original_namesstore that the disk-fallback branch does — that store exists for when the resolved disk path differs from the requested specifier, which cannot happen for a map hit (the map is keyed by the specifier). The no-plugin file-map path never stored it either. - The refactor deduplicates rather than duplicates: one helper called from two sites, replacing an inline block at one and adding the missing lookup at the other, per the repo's "fix the whole class" guidance.
…t --splitting (#41263) ### Problem - Without `--splitting`, `bun build --outdir out ./a.ts ./b.ts` silently writes only `out/b.js` when the files import each other. CSS entry points lose a file the same way. A `--compile` executable can run nothing. - Cause: `compute_chunks` keys an entry chunk by the full `entry_bits` of its file (`src/bundler/linker_context/computeChunks.rs:120`). Entry points that reach each other get equal keys, so the second overwrites the first. - Same root: when entry `a` imports entry `b`, `out/a.js` can run `b`'s imports too early. ### Fix - Key each entry chunk and CSS entry by the entry point's own bit, as code splitting does (#26089). `LinkerGraph::load` keeps one entry point per file. - Without code splitting, `find_imported_parts_in_js_order` starts its walk at the chunk's own entry point. HTML scripts now run in page order. - In cjs output, only the entry point of a chunk writes `module.exports`, as in esbuild. This also fixes a re-export leak in single-entry builds. A missing ESM wrapper is no longer called (`__INVALID__REF__();`). - Verified: 11 new tests in `test/bundler/`. 9 fail on the base. Self-review: 3 concerns, all addressed. ### Background - `entry_bits` is the set of entry points that reach a file. - Without code splitting, Bun links all entry points in one pass (esbuild links each alone). An entry chunk holds every file its entry point reaches. - A chunk prints files in the order of an import walk. It starts at the smallest `distance_from_entry_point`, which is 0 for every entry point. <details><summary>Notes</summary> **Repro** ```ts // a.ts import { b } from "./b.ts"; export const a = "a"; console.log("a sees", b); // b.ts import { a } from "./a.ts"; export const b = "b"; console.log("b sees", a); ``` `bun build --outdir out ./a.ts ./b.ts` prints only `b.js (entry point)`. With `--splitting`, both files are written. Two CSS files that `@import` each other lose one output, with and without `--splitting`. For `--compile`: `main.ts` does `import("./plugin.ts")` and `plugin.ts` does `import("./main.ts")`. `bun build --compile ./main.ts ./plugin.ts --outfile app` makes an executable whose main module is the chunk of `plugin.ts`. The code of `main.ts` sits in an `init_main` wrapper that nothing calls, so `./app` prints nothing and exits 0. **Module order** `a.ts` imports `c.ts`, then `b.ts`. `b.ts` imports `d.ts`. Both `a.ts` and `b.ts` are entry points. Unbundled, `bun a.ts` prints `c d b a`. On 1.4.1, `out/a.js` prints `d b c a`: `b.ts` also has distance 0 and a lower stable index, so the walk starts there. esbuild prints `c d b a`. An HTML page with `<script src="./first.js">` and `<script src="./second.js">` runs `second` first when `second.js` is also an entry point. With only the key change, the cycle's `out/a.js` would get the module order of `out/b.js`. So the key change and the walk change stay together. **`module.exports` in cjs output** Two writes to `module.exports` come from the linker, not from user code: - Step 5 puts `module.exports = __toCommonJS(exports)` at the end of part 0 of each entry point file (`doStep5.rs`). - `export * from` a module that needs a run-time re-export prints `__reExport(exports, mod, module.exports)` (`convertStmtsForChunk.rs`). Both are per file, and the file of one entry point can print in the chunk of another. The walk emits part 0 of a file when it enters the file, before its imports. On 1.4.1 the other entry point's file came first, so its writes came before the chunk's own assignment and did no harm. With the walk change, they come last. For `index.ts` that imports `utils.ts`, both entry points, `require("./out/index.js")` returned the exports of `utils.ts`. In a cycle, 1.4.1 already got this wrong: its one output, `b.js`, exported the names of `a.ts`. Now both writes happen only when the file is the entry point of the chunk (`Chunk::is_entry_point_file`). esbuild has the same rule (`file.IsEntryPoint()`, in a link of one entry point). The part 0 write is the last statement of that part, and the printer drops it in other chunks. The re-export rule also changes single-entry builds. On 1.4.1, a file that the entry point imports, but does not re-export, still wrote its `export * from "ext"` names onto `module.exports`. esbuild does not. A re-export chain still works, because each file in it re-exports the next file at run time, and the entry point file writes to `module.exports` (checked with a chain of three files). `importstar/ReExportStarEntryPointAndInnerFileExternal` expected the leak. Before 2f7ff95, the harness threw for every cjs test, and this test expected the value that esbuild prints. That commit set the current value when cjs output first ran. The same commit added `ReExportStarEntryPointAndInnerFile`, without externals, which expects no top-level `b`. This PR restores the esbuild value. **Two entry points that name one file** An onResolve plugin can map two entry point names to one file. The plugin path adds the file to the entry points twice. On 1.4.1, the full-bits key merged the two chunks without `--splitting`, so the build wrote `second-name.js`. With `--splitting`, it failed with "Multiple files share the same output path". `LinkerGraph::load` now skips a file that is already an entry point, as its loop for `import()` targets does. Both modes write `second-name.js`. The name comes from `entry_point_original_names`, which keeps the last name. When a plugin declines an entry point and the default resolver finds a file that is already an entry point, `enqueue_entry_item` returns `None` and records no name. This PR does not change that path. #38600 works on it. **Output order** The old non-split key started with a byte that was 0 for an HTML entry point and 1 for the others. It sorted HTML entry chunks before JS entry chunks. The own-bit key has no such byte. For `bun build ./index.html ./b.ts ./c.html`, the outputs are now listed as `index.html`, `b.js`, `c.html` (with their chunks), in the order of the entry points. The files and their contents do not change. Entry point 0 still sorts first, so `--compile` picks the same main module. **Tests** New, and failing on the base commit: - `edgecase/EntryPointsImportEachOther`, `edgecase/EntryPointImportsEntryPointModuleOrder`, `edgecase/CSSEntryPointsImportEachOther`, `edgecase/EntryPointsImportEachOtherCommonJS` - `html/script-that-is-also-an-entry-point` - `compile/EntryPointsImportEachOther` - `plugin/two entry points that resolve to one file (splitting: true)` - `edgecase/EntryPointRequiredByEntryPointCommonJS`, `edgecase/RequiredEntryPointWithoutWrapperCommonJS` New, and passing on the base commit: `edgecase/EntryPointImportsEntryPointCommonJS` and the `splitting: false` plugin case. Both fail if either cjs write or the dedupe is removed. Changed: `importstar/ReExportStarEntryPointAndInnerFileExternal` (above) and `compile/HelloWorldWithProcessVersionsBun`. A debug build inlines `process.versions.bun` as `1.4.1-debug`. That test removed `-debug` only from the expected value, so it failed on every debug build. The API variant of the test removes it from both values, and now this one does too. **Checked by hand** Against esbuild 0.25.1 (run output and `module.exports` compared): a ring of three entry points in `esm`, `cjs`, and `iife`, a CommonJS cycle, `export * from` another entry point, re-export chains of two and three files in cjs, and a named import from a file that re-exports a `node:` module. Also rings of 10 and 130 entry points (130 uses the heap bit set), `Bun.build` with `metafile` and `sourcemap: "external"`, an HTML entry point whose script is in a cycle of two JS entry points, and `--no-bundle --format=cjs` (byte-identical output). **Suites run with the debug build** `bundler_edgecase`, `bundler_html`, `bundler_html_server`, `bundler_plugin`, `bundler_cjs`, `bundler_compile`, `bundler_compile_splitting`, `bundler_splitting`, `esbuild/{default,importstar,importstar_ts,dce,splitting,css,metafile,loader,lower,ts,extra,packagejson}`, `metafile`, `bundler_naming`, `html-import-manifest`, `bundler_cjs2esm`, `bundler_regressions`, `bun-build-api`, `bundler_browser`, `bundler_bun`, `cli`, `bundler_loader`, `bundler_files`, `bundler_barrel`, `bundler_minify`, `regression/issue/5344`, and `bake/{dev/production,dev-and-prod,dev/ssg-pages-router}`. An earlier round also ran `bun-build-compile`, `compile-asset-bunfs`, `native-plugin`, and `regression/issue/{10004,10139,22317,25716,27431,32686}`. Some tests time out at 5 s on the debug build. Each of them builds one entry point, and each passes with a longer timeout. **`__INVALID__REF__();` in cjs output** A file that something `require()`s gets an ESM wrapper (`init_foo`). When all of its top-level statements hoist, the parser gives it no wrapper symbol (`needs_wrapper_ref`), and the printer prints the file with no wrapper. The esm tail of an entry point checks for the symbol. The cjs tail did not, so it printed `__INVALID__REF__();`, and loading the output threw `ReferenceError: __INVALID__REF__ is not defined`. The cjs tail now has the same check. One entry point is enough: `entry.ts` has `export function load() { return require("./c.ts"); }`, and `c.ts` requires `entry.ts`. With two entry points, `a.ts` does `require("./b.ts")`, and `b.ts` has only `export const` lines. 1.2.0, 1.3.0, 1.4.0, and 1.4.1 all print the bad call. #37843 has the same check, and it also adds the missing iife tail. `bun build --format=iife` of a CommonJS entry point runs none of its code (1.2.0 through main), because nothing calls `require_entry()`. This PR does not change iife output. **Related** - #30544 made the same key change in the Zig source. It was closed because the Rust port removed those files. - #40601 changes where an entry point's module goes with `--splitting`. It does not touch the non-split key. - #41235 changes how `import.meta.main` prints when a build has several entry points. - #37843 adds the iife tail and has the same cjs check as this PR. </details>
|
Closing in favor of #45408. It gives a declined |
1 participant
Problem
Bun.build({ entrypoints: ["/v/page.html"], files: { "/v/page.html": ..., "/v/app.js": ... } })builds, but adding any onResolve plugin whose filter matches the entry point and whose callback returnsundefinedmakes the same build fail withModuleNotFound resolving "/v/page.html" (entry point). Same for.jsentry points; the loader is irrelevant.enqueue_entry_points_normal(src/bundler/bundle_v2.rs) hands an entry point to the plugins before it consultsself.file_map. When the plugin declines,on_resolve'sNoMatcharm forImportKind::EntryPointBuild(previouslybundle_v2.rs:4655) calledresolve_entry_pointdirectly, i.e. disk resolution, and never looked at thefilesmap. The equivalentNoMatchfallback for imports,run_resolver, does check the map first, so only entry points were affected.filesoption (feat(bundler): addfilesoption for in-memory bundling #25852) added its lookup to the no-plugin entry path,run_resolver, andresolve_import_records, but not to this arm, which the earlier virtual-entry-point plugin support (fix: support virtual entrypoints in onResolve() #22144) had added.Fix
BundleV2::enqueue_entry_point_from_file_map: looks the entry point up in thefilesmap and, on a hit, enqueues it throughenqueue_entry_itemwith the requested target. It replaces the inline lookup inenqueue_entry_points_normaland is called from theNoMatcharm before the disk fallback.filesmap, then disk. That is already the order used for imports (plugin, thenrun_resolver's map lookup, then the resolver), and it is what the no-plugin path did for entry points. A plugin that declines is therefore a no-op, which is the contract onResolve has for every other specifier.enqueue_entry_itemcall as the no-plugin path (same target, same output naming), so the outputs are byte-identical with and without the declining plugin; a miss still falls through to the unchanged disk path and its existing error.file_map, so the helper is a no-op there.test/bundler/bundler_files.test.ts(newdescribeblock): JS and HTML virtual entry points with a catch-all declining plugin, an asynchronously declining plugin, byte-for-byte equality with a plugin-free build, a build mixing a virtual and an on-disk entry point, and an entry point missing from both the map and disk still reportingModuleNotFound. 6 of the 7 new tests fail on the released binary (USE_SYSTEM_BUN=1), all 30 tests in the file pass withbun bd test.bundler_plugin.test.ts,bundler_plugin_chain.test.tsandregression/issue/bundler-plugin-onresolve-entrypoint.test.tswith the debug build: no failures.Background
filesoption /FileMap:Bun.build({ files })supplies in-memory sources keyed by path. The bundler consults this map (FileMap::resolve) before touching the disk wherever it resolves a specifier; a hit yields a resolver result in thefilenamespace whose contents are later read from the map instead of the filesystem.Resolverequest of kindEntryPointBuildand sends it to the plugin on the JS thread. The answer comes back toBundleV2::on_resolveasSuccess(plugin supplied a path),Err, orNoMatch(every callback returned nothing).NoMatchis supposed to behave as if no plugin existed.enqueue_entry_item: registers a resolved entry point in the graph (path to source index map, input file, parse task) and appends it tograph.entry_points; both the no-plugin path and theNoMatcharm end up here.Repro