bundler: keep the import kind of server-side HTML imports so import() and require() of the manifest link - #38621
bundler: keep the import kind of server-side HTML imports so import() and require() of the manifest link#38621robobun wants to merge 3 commits into
Conversation
… and require() of the manifest link
resolve_import_records() overwrote the importing record's kind with
ImportKind::HtmlManifest when binding a server-side HTML import to its
generated manifest module. The linker and printer decide how to emit an
import from that kind: import() and require() records get the imported
module wrapped (or, with splitting, turned into a chunk of its own) and
are printed as a call to that wrapper. With the kind replaced, the
manifest module was never wrapped, and import("./page.html") or
require("./page.html") printed a call to an undefined require_page().
Only the record that created the manifest module was rewritten, so the
result also depended on which importer was resolved first.
The kind is now left alone; the only thing that read the rewritten kind
was the known_target of the HTML file's parse task, which uses the same
predicate directly.
The manifest module's AST also gets the server target it is evaluated
under. It defaulted to browser, so with splitting enabled the chunk it
lands in (a chunk of its own for import(), or a chunk shared between
server entry points for static imports) was flagged as a browser chunk
of the server build and listed in the manifest as one of the page's
files.
|
Warning Review limit reached
Next review available in: 55 seconds 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 on bun 1.4.0 with a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The fix is small and well-reasoned, but it changes load-bearing bundler linking behavior (import-record kind drives wrapping/chunking decisions) and has a user-observable side effect on metafile kind reporting, plus noted interactions with #38605 and #38347 — a maintainer familiar with the bundler should sign off.
Checked that the known_target rewrite from import_record.kind == HtmlManifest to is_html_entrypoint is equivalent (the removed assignment was the only setter, 20 lines above). Checked remaining HtmlManifest consumers: LinkerGraph.rs:947 (TLA propagation) now falls through for static-import manifest records, which is harmless — the manifest is a lazy-export with no TLA, same as a JSON import. Verified bunRun returns {stdout, stderr, exitCode} with trimmed strings, so the two-line stdout.split("\n") in the dynamic-import test is sound.
Extended reasoning...
Overview
Two changes in src/bundler/bundle_v2.rs: (1) stop overwriting import_record.kind with ImportKind::HtmlManifest in resolve_import_records() and use the local is_html_entrypoint predicate directly for known_target; (2) set the generated manifest module's AST target to the importing server target in generate_server_html_module() instead of leaving it at the Browser default. Five new tests in test/bundler/html-import-manifest.test.ts cover import(), import() + splitting, require() from ESM and CJS, and a shared manifest chunk under splitting.
Security risks
None. This is bundler output-shape logic; no untrusted input parsing, auth, or crypto is touched.
Level of scrutiny
High. The line count is tiny (~8 native lines), but import_record.kind is load-bearing across the linker (scanImportsAndExports.rs), chunk computation (bundle_v2.rs:1852), and printer (js_printer/lib.rs) as the PR description itself lays out. The change also alters user-observable metafile output (html_manifest → import-statement/dynamic-import/require-call). I audited every remaining reader of ImportKind::HtmlManifest — only LinkerGraph.rs:947 (TLA-propagation continue) is behaviorally affected, and the fallthrough for a static-import record just visits a lazy-export module with no async dependency, matching how JSON imports are already handled. The known_target rewrite is trivially equivalent since the deleted assignment was its only setter.
Other factors
The tests are well-constructed: they run the bundled output with bunRun, assert stderr and manifest shape before exitCode, use tempDir with await using, and pin the metafile kind. The PR description explicitly flags interactions with two open PRs (#38605 would reintroduce the same assignment on other paths; #38347 overlaps the splitting symptom) — a maintainer should coordinate the landing order. Given the subsystem, the observable metafile change, and the cross-PR coordination, this warrants a human sign-off despite finding no defects.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of any of the three; checked each:
The main change in this PR (not rewriting the record kind, which is what the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The Rust change is small and the reasoning is thorough, but since it changes how HTML-manifest import records flow through the linker (with a user-visible metafile kind change) and overlaps with three open PRs that take different directions on the same code, a human look would still be worthwhile.
Checked: is_html_entrypoint is computed just above and is equivalent to the removed kind == HtmlManifest check; remaining ImportKind::HtmlManifest readers (LinkerGraph.rs:947 async-propagation walk, ResolveMessage.rs/ast/lib.rs label tables) are unaffected — the Stmt arm now visits the manifest module like any JSON import, which is harmless. No test in the tree asserts on "html_manifest" as a metafile kind. The new tests use bunRun/tempDir correctly and expectPageManifest asserts the exact object shape.
Extended reasoning...
Overview
The PR touches two files: a ~6-line net change in src/bundler/bundle_v2.rs (stop rewriting import_record.kind to ImportKind::HtmlManifest in resolve_import_records(), use the is_html_entrypoint local for known_target instead, and set the generated manifest module's AST target to the importing server target in generate_server_html_module()), plus five new tests in test/bundler/html-import-manifest.test.ts covering import(), require() (ESM/CJS), and splitting variants.
Security risks
None. This is bundler output-shape logic; no auth, crypto, untrusted-input parsing, or filesystem-escape surface is touched.
Level of scrutiny
Medium-high. The diff itself is tiny and mechanically sound — I traced is_html_entrypoint (defined at bundle_v2.rs:6548) and confirmed it's exactly the predicate the removed assignment encoded, and I audited every remaining reader of ImportKind::HtmlManifest (the async-dependency walk at LinkerGraph.rs:947 now falls through to Stmt for static imports, which just visits a synchronous lazy-export module — same as a JSON import — and still skips for Dynamic/Require; the other hits are label tables). But the change sits inside the bundler's link/print decision path, has a documented user-visible side effect (metafile kind for HTML imports changes from html_manifest to the real syntax kind), and the PR description itself notes it conflicts directionally with #38605 and overlaps with #38376/#38347/#38549. That coordination is a maintainer call.
Other factors
The PR description is unusually detailed with a proven mechanism (not just "crash goes away"), the five new tests each pin a distinct failure mode (wrapper-not-defined, chunk-not-emitted, wrong result shape, server chunk leaking into manifest), and existing tests in the same file are unchanged. bunRun trims stdout so stdout.split("\n") in the dynamic-import test yields the expected two lines. The duplicate-PR bot flagged three overlapping PRs, one of which (#38605) adds the very assignment this PR removes — a human should decide which direction wins before either lands.
1 participant
Problem
target: "bun"(ornode) build,await import("./page.html")builds successfully but the output calls a wrapper that is never defined, and fails at startup withReferenceError: require_page is not defined. Same forrequire("./page.html")from an ESM file. Reproduces on 1.4.0, with and withoutsplitting.require("./page.html")from a CommonJS-style file evaluates to{ default: manifest }instead of the manifest, and withsplitting: truea manifest module shared by two server entry points gets its (server) chunk listed in the manifest as one of the page's browser files.resolve_import_records()(src/bundler/bundle_v2.rs:6562) overwrote the importing record'skindwithImportKind::HtmlManifestwhen it bound the record to the generated manifest module. The linker and printer decide how to emit an import from the record's kind:Require/Dynamicrecords get the imported module wrapped (scanImportsAndExports.rs:225-248) or, with splitting, turned into a chunk of its own (bundle_v2.rs:1852), and are printed as a call to that wrapper, aPromise.resolve().then(...)around it, or animport()of the chunk (js_printer/lib.rs:2506-2624).HtmlManifestmatches none of those arms, so the manifest module was never wrapped while the printer still emitted the wrapper call (require_or_import_meta_for_sourcereturns the wrapper ref unconditionally).path_to_source_index_mapearly return and kept its real kind. So a secondimport()of the same page worked and the first did not, and the metafile reportedhtml_manifestfor one importer anddynamic-importfor the other.splittingsymptom):generate_server_html_module()never set the manifest module's AST target, so it kept theto_astdefault ofBrowser.computeChunksuses the per-file target to flag chunks of a server build as browser output, andHTMLImportManifestlists such chunks in the manifest.Fix
known_targetof the HTML file's parse task, which now uses theis_html_entrypointpredicate directly. Nothing setsImportKind::HtmlManifestany more; the variant itself is left in place since bundler: label conditional CSS @import records as import-rule #38549 is realigning the kind tables around it.generate_server_html_module()..jsonfile parses to), evaluated by server code; what differs between a static import,import()andrequire()of it is exactly what the record kind tells the linker. With the kind intact it goes through the same paths as a.jsonimport and produces the same shapes: a static import staysvar page_default = __jsonParse(...)(output unchanged for the existing tests),import()yields a promise of a namespace whosedefaultis the manifest, andrequire()yields the manifest itself. That also matches what the unbundled runtime does (require("./page.html")is theHTMLBundle,import()puts it underdefault), and it makes every importer of a page behave like the second importer already did.import-statement,dynamic-import,require-call) for HTML imports.html_manifestwas never part of the declaredImportKindtype, was only reported for the first importer, and predates the metafile (Introduce ahead of time bundling for HTML imports withbun build#20265 vs feat(bundler): add metafile support matching esbuild format #25842, which matches esbuild's format, wherekindis the import syntax).test/bundler/html-import-manifest.test.ts:dynamic-import(also pins the metafile kind),dynamic-import-with-splitting,require-from-esm,require-from-cjs,splitting-shared-manifest-chunk. All five fail on 1.4.0 (the first three withrequire_page is not definedor noimport()emitted, the last two on the shape of the result), all pass with this change; the file's six existing tests still pass.bundler_html_server,bundler_html,metafile,regression/issue/28042,bun-serve-html-manifest, and the HTML import case ofbundler_compile; a manualbun build --compileof a file thatimport()s a page runs.kind = HtmlManifestassignment to the plugin andfiles:resolution paths, which would carry this bug onto those paths; whichever of the two lands second should drop it (noted there). bundler: give the browser side of a server build its own copy of the runtime #38376 (browser runtime copy) contains the same one-line target fix; the hunk here is identical to it, sobundle_v2.rsmerges cleanly in either order, and this PR adds the test for the symptom that line fixes on its own (splitting-shared-manifest-chunk). bundler: list the chunks and assets reached through dynamic imports in the HTML import manifest #38347 changes how the manifest picks chunks and would hide that symptom, but the chunk would still be named and sided as browser output, so the target fix stands on its own. The kind fix is in none of them.Background
.htmlfile, the HTML (with its scripts and styles) is bundled as a separate browser build, and the import evaluates to a JSON object listing the files that build produced (Bun.serveroutes consume it). Internally the import record is bound to a generated "manifest module": a lazy-export module whose body is__jsonParse("<placeholder>"); the linker splices the JSON in once output paths are known..json,.toml,.txtfiles parse to one). The linker finalizes its shape after it knows how the module is imported:export default <expr>for static imports,module.exports = <expr>inside a__commonJSwrapper when it isrequire()d orimport()ed without splitting.require()d,import()ed without splitting, or CommonJS), the bundler emits it as a function (require_<name>/init_<name>) and prints the importing expression as a call to it. Whether a module gets a wrapper is decided from the kinds of the records that import it, which is why the kind is load-bearing.Browser.computeChunksuses this to mark browser chunks of the server build, which get browser naming,side: "client", and an entry in the manifest.