Repository navigation
Conversation
…runtime A server-side build that imports HTML files bundles two module graphs, one per target, but parsed a single copy of the bundler runtime (runtime.js), with the server flavor. It was the only source shared by both graphs, so every browser chunk took its helpers from a runtime built for Bun (`__require = import.meta.require`), the server entry carried the helpers only the browser used and vice versa, and with --splitting the runtime landed in one chunk that both the server entry and the browser chunks imported: a `// @bun` file listed in the HTML import manifest and loaded by the browser. The build now parses a second copy of the runtime for Target::Browser when it creates its client transpiler, and browser-target files bind their runtime imports to it. On the linker side every place that handed out a runtime helper or special-cased the runtime source index goes through LinkerGraph::runtime_source_index_for / runtime_function_for / is_runtime_source, so each side's files depend on, print and import the helpers of their own copy; chunk assignment then keeps the two copies apart on its own. The parser learns which source is a runtime copy from an explicit option instead of assuming source index 0. Bake builds bring their own client transpiler and are unchanged.
|
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 (18)
Comment |
|
Status: reproduced and fixed; waiting on CI.
|
… bundler print_with_writer decided whether to print a file as a dev server module by testing for source index 0. The browser copy of the runtime is not at index 0, so pass the bundler's notion of which sources are runtime copies in through the print options, like the parser and linker already do.
The manifest's input field is not a plain relative name on Windows, so the new runtime tests select entries by loader/isEntry and compare file names.
There was a problem hiding this comment.
Re-reviewed after 4dc4126 — the test now matches manifest entries and output files by basename(path) and picks the client chunk via loader === "js" && isEntry, which addresses the Windows path-matching failure I flagged. No new findings this pass. Given the scope — a new browser_runtime_source_index invariant threaded through ~15 linker sites plus parser/printer option plumbing across 18 files — a human look at the linker changes is still worthwhile.
Extended reasoning...
Overview
This PR fixes server builds that import HTML files: previously both the server and browser module graphs shared a single copy of the bundler runtime parsed for the server target, so browser chunks got var __require = import.meta.require (and other server-flavored helpers) and, with --splitting, the runtime landed in one // @bun-tagged chunk imported by both sides. The fix enqueues a second runtime copy parsed for Target::Browser when ensure_client_transpiler first runs, records its source index on Graph/LinkerGraph, and routes every runtime-helper lookup through runtime_function_for(file, name) / runtime_source_index_for(file) / is_runtime_source(idx). The parser and printer gain source_is_runtime so the second copy is parsed/printed like the first instead of being inferred from index 0. Touches 18 files across src/bundler/, src/js_parser/, src/js_printer/, plus three new tests in test/bundler/html-import-manifest.test.ts.
Since my last review
Commit 4dc4126 reworked the new tests to key everything by basename(...) and to select the client chunk structurally (loader === "js" && isEntry) rather than by the manifest input string, which was the cross-platform fragility I raised. The outputs map is now keyed by basename(output.path) and manifest entries carry name: basename(f.path), so the Map lookups line up regardless of how the manifest spells its paths. This looks like it resolves the Windows failure; build #95643 is still running per robobun. My earlier printer nit (the source.index.0 != 0 check) was already addressed in 7bd8c32 and is reflected in the diff (js_printer::Options::source_is_runtime).
Security risks
None identified. This is bundler output-shaping logic with no auth/crypto/permission surface; inputs are the same source files the bundler already processes.
Level of scrutiny
High. The change is architecturally clean (removes a special case rather than adding one), but it replaces a hard-coded Index::RUNTIME at many independent linker sites — scanImportsAndExports, doStep5, convertStmtsForChunk, generateCodeForFileInChunkJS, generateCompileResultForJSChunk, postProcessJSChunk, findAllImportedPartsInJSOrder, MetafileBuilder, plus LinkerContext::create_wrapper_for_file and the HTML manifest __jsonParse binding. A missed site would produce wrong output only in the server-build-with-HTML-import combination, which is exactly the coverage gap that let the original bug ship. The PR description is thorough and lists an extensive set of passing test files, but this is core linker code where a maintainer should confirm the design (per-file runtime lookup vs. per-chunk, and the ast.target read in runtime_source_index_for) and eyeball the site enumeration.
Other factors
The bug-hunting pass found nothing new this run. The comment-cop lint comments are all resolved (3a50878). The one open item is CI #95643 confirming the Windows test fix; beyond that, nothing blocks from my side, but the size and location of the change put it outside what I'd auto-approve.
Problem
--target=bun(ornode) build that imports an HTML file bundles two module graphs, one per target, but parsed one copy of the bundler runtime (runtime.js), with the server flavor. It was the only source shared by both graphs.var __require = import.meta.require;is emitted into the JS a page loads (the browser flavor is a shim around a globalrequire), and__using/__callDisposecome without their browser fallbacks. This happens with or without--splitting.__commonJS, ...) and the browser chunks carried the server's__jsonParse.--splitting, the runtime was reachable from the server entry and from the browser entries, socompute_chunks(src/bundler/linker_context/computeChunks.rs, the "which JS files are in which chunk" loop) keyed it by that combined bit set: one chunk, starting with// @bun, thatserver.jsimports__jsonParsefrom, that every browser chunk imports__requirefrom, and that is listed in the HTML import manifest, i.e. served to the browser.enqueue_entry_points_common(src/bundler/bundle_v2.rs) parsed the runtime once for the main target,resolve_import_recordsbound every file's runtime imports toIndex::RUNTIME, and the linker's helper lookups (runtime_function,cjs_runtime_ref,generateCompileResultForJSChunk.rs,postProcessJSChunk.rs, ...) all hardcodedIndex::RUNTIME.Fix
ensure_client_transpiler(the point where a server build starts bundling browser code, for an HTML import or an HTML entry point) now also enqueues a second copy of the runtime parsed forTarget::Browser, recorded inGraph::browser_runtime_source_index.enqueue_entry_points_commonand this path shareenqueue_runtime.resolve_import_recordsbinds a file's runtime import records (the parser'sbun:wrapimport and user-writtenimport ... from "bun:wrap") to the copy for the file's target.LinkerGraph::runtime_source_index_for(file)picks the copy from the file'sast.target; every place that used to look up a helper inIndex::RUNTIMEnow goes throughruntime_function_for(file, name)/generate_runtime_symbol_import_and_use, and dependencies on the helper's parts use the ref's own source index. The printer's interop refs (__toESM,__toCommonJS,__require) are taken per part range / per chunk (runtime_print_refs_for), since a chunk's code must name the symbols of its own side. The "is this the runtime" special cases (never wrap it, print its parts first, no path comment, no source map, not a metafile input, skip TLA validation) useis_runtime_source, andfind_reachable_filesvisits the second copy like the first.generate_server_html_modulegets the importing side's target (the parser leavestargetat the browser default), soruntime_source_index_forand the existing browser-chunk flagging incompute_chunkssee it as a server file.ParserOptions::source_is_runtime(set from the newParseTask::is_runtime) instead of inferring "this is the runtime" from source index 0; this is what keepsrequireuntouched inside the runtime's own__requireshim and disables helper imports into the runtime, so the second copy is parsed exactly like the first.--format=internal_bake_dev, also used by the bake dev server) prints as a module; it now getsjs_printer::Options::source_is_runtimefrom the linker instead. For the first copy this is the same decision as before (test/bake/dev/{esm,bundle}.test.tsand theinternal_bake_devtests inbundler_loader.test.tspass); the second copy only shows up in that format together with an HTML file, which already trips a pre-existingdebug_assertinprint_dev_server_moduleon main, so that combination is not testable here and is reported separately.path_to_source_index_map; the runtime was the one exception. Once each side's files depend on their own copy, entry bits, chunk assignment, cross-chunk imports, the// @bunpragma, chunk naming and the manifest all come out right without special cases, and the browser copy is the onebun build --target=browserwould have produced (the shared browser chunk in the repro is byte-identical to the runtime chunk of a plain browser build, same content hash). With no browser side,browser_runtime_source_indexis invalid and every helper resolves toIndex::RUNTIMEas before.ensure_client_transpilernever takes the branch, and it keeps the single shared runtime chunk thatbake::productionwrites out for the client itself.[name]of the shared browser chunk is still the first entry point's (server-<hash>.jsfrom the CLI,chunk-<hash>.jsfrom the API /--compile); that is how every shared split chunk is named today and is tracked separately in bundler: stop naming shared split chunks after the first entrypoint #36326.test/bundler/html-import-manifest.test.ts: three new tests (no splitting: browser chunk has the browser__requireand no__jsonParse, server entry has no__commonJS; splitting: the only shared chunk is browser-flavored andserver.jsimports nothing the manifest lists; two HTML imports with splitting: the shared runtime chunk is in both manifests and has no// @bun). They fail on the release build withvar __require = import.meta.require;in the browser chunk and// @bunon the shared chunk, on Linux and on Windows (checked against a Windows x64 build of this branch as well; the tests select manifest entries by loader/isEntry and compare file names, since the manifest'sinputfield is not a plain relative name on Windows today). The existingmanifest-with-metadatainline snapshot changes because the browser chunk no longer contains the server's__jsonParse, which moves its content hash and the HTML etag.test/bundler/{bundler_html,bundler_html_server,bundler_splitting,bundler_compile_splitting,bundler_bun,bundler_browser,bundler_cjs,bundler_cjs2esm,bundler_naming,metafile,bundler_edgecase,bundler_regressions,bun-build-api,bundler_plugin,standalone,compile-asset-bunfs}.test.ts,test/bundler/esbuild/{splitting,default}.test.ts,test/bundler/transpiler/transpiler.test.js,test/js/bun/http/bun-serve-html{,-manifest}.test.ts, the HTML-import regression tests intest/regression/issue,test/bake/dev-and-prod.test.tsandtest/bake/dev/{esm,bundle,html,production}.test.ts;cargo clippyonbun_bundler,bun_js_parserandbun_js_printerandcargo fmt --checkare clean.--target=node(server gets thecreateRequireflavor, browser its own), an HTML file passed directly as an entry point next to the server entry,--minify --splitting(both sides still run),--compile --splitting(the compiled server serves the browser entry and the runtime chunk it imports, now a browser chunk), and the metafile.Background
src/runtime.jsplus a per-target tail inParseTask.rs) holding the helpers bundled code calls, such as__commonJS,__toESM,__require,__jsonParse. It is parsed as source index 0 (Index::RUNTIME). Files reach it in two ways: the parser emits an internalimport {...} from "bun:wrap"for the helpers it uses, and the linker adds part dependencies for the helpers it introduces itself (wrappers,__export, the HTML manifest's__jsonParse).import page from "./index.html"in--target=buncode makes the bundler build the HTML file and everything it references as a browser-target graph in the same build, and replaces the import with a manifest (the list of output files, paths and etags) thatBun.serveuses to serve them. Each source file is parsed once per target (build_graphs[target]), so the two graphs share no files.--splitting): every file is marked with the entry points that reach it; files with the same set of entry points form one chunk, so a file reached by several entries becomes a shared chunk those entries import. Whether a chunk is server or browser code (pragma, naming, public path, manifest listing) is decided from the target of the file the chunk was created for.Repro output before / after
Before,
out/server-08y1dfjd.js(imported byserver.js,client-*.jsandlazy-*.js, listed in the manifest):After,
server.jsstarts with// @bun\nvar __jsonParse = ...and imports nothing; the chunk the browser files import iswhich is byte-for-byte the runtime chunk
bun build client.js --target=browser --splittingemits (samec3taa3cghash).Without
--splitting, the browser chunk of a client that imports a CommonJS file and callsrequire()went from containingvar __jsonParse = ...; var __require = import.meta.require;to the browser__requireshim and no__jsonParse; the server entry went from 1.81 KB (every helper the browser used) to 0.61 KB.