bundler: copy HTML url assets resolved through files: or an onResolve plugin - #38635
bundler: copy HTML url assets resolved through files: or an onResolve plugin#38635robobun wants to merge 1 commit into
Conversation
… plugin The rule that copies a url asset referenced from an HTML document to the output even when its extension has a parsing loader (json, toml, ...) was applied only when the reference was resolved by the resolver on the bulk resolution path. A reference that hit the in-memory file map, that the resolver handled after an onResolve callback declined it, or whose path came from an onResolve callback kept the parsing loader, so the file was bundled as a module and the reference in the HTML was left as written. Move the rule into loader_for_import and apply it on every path that picks the loader for an import record. The in-memory branch of resolve_import_records, a copy of the disk tail that had drifted, is removed: a file map hit now stands in for the resolver's result and goes through the same tail, keeping the jsx options from the transpiler and the display path of the map key.
|
Warning Review limit reached
Next review available in: 35 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 (3)
Comment |
|
Status: fix and tests pushed, waiting on CI. Reproduced on bun 1.4.0 with a page referencing The branch is based on d4ccab4 because main at 2f5c180 does not compile ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it deletes the in-memory branch of resolve_import_records and routes those files through the disk tail — which newly exposes them to the dev-server cache check, is_html_entrypoint, and secondary_path_for_commonjs_interop — and because it overlaps with three open PRs on the same lines, a human look would be worthwhile before merging.
What was reviewed:
loader_for_plugin_importguardsEntryPointBuildbefore indexingitems_loader, so theu32::MAXsentinel is never dereferenced; the only otherMiniImportRecordproducer sets a valid parsed-file index.FileMap::result_for_keyreturnsDefault(HasSideEffects, not external, no secondary path), so the tail'sis_external/ secondary-path handling is a no-op for in-memory results andParseTask::initpicks up the sameside_effectsthe deleted branch set.- The
if let Some(mut result) = in_memory_resultinside'inner: loopis fine — theSomearm alwaysbreaks, and theNonearm doesn't move the option, so thecontinue 'innerretry path still sees it. - The rule in
loader_for_importis byte-identical to the inline block it replaces; disk-no-plugin builds are unaffected.
Extended reasoning...
Overview
This PR extracts the "HTML url references get the file loader even if their extension has a parsing loader" rule from an inline block in the disk tail of resolve_import_records into loader_for_import(), and applies it at all four places that pick an import record's loader: the bulk resolver tail, both branches of run_resolver (onResolve declined), and on_resolve (onResolve returned a path). It also deletes the ~60-line in-memory (Bun.build({ files })) branch of resolve_import_records and instead lets a file-map hit stand in for the resolver's result at the top of the loop, so in-memory files flow through the same tail as disk files. Nine new tests cover each resolution path plus regression guards for jsx options and import attributes on the removed branch.
Security risks
None. This is bundler control flow deciding whether a referenced file is copied verbatim or parsed as a module; no untrusted input parsing, auth, crypto, or filesystem-boundary changes.
Level of scrutiny
High. resolve_import_records is the core of the bundler's module-graph construction, and collapsing a parallel branch into a shared tail means every side effect of that tail now applies to in-memory files. I traced each field the deleted branch set (known_target, jsx + force_node_env, loader, tree_shaking, side_effects) to its equivalent in the shared tail via ParseTask::init and FileMap::result_for_key's defaults — they match. I also checked what the tail does that the deleted branch didn't: the is_external() check and secondary_path_for_commonjs_interop are no-ops for the default Result; the dev-server cache block is gated on self.dev_server which is None for Bun.build; and is_html_entrypoint is the HTML-import-manifest behavior the PR intentionally enables and tests.
Other factors
The PR description explicitly maps out conflicts with three open PRs (#38605, #38620, #38609) that touch these same lines. That coordination, plus the fact that this is a non-mechanical refactor of a hot bundler path with CI still pending, is why I'm deferring rather than approving. The change itself reads correct and is exceptionally well-tested (each resolution path has its own test, plus guards for the two behaviors preserved from the removed branch), but the merge-order decision and confirmation that the newly-shared tail doesn't regress any dev-server / bake interaction with file_map warrants a maintainer's eyes.
|
On the dev server question: |
1 participant
Problem
<link rel="manifest" href="./manifest.json">, json/toml/yaml/text/...) only gets the asset copied to the output when the reference is resolved by the resolver on the bulk resolution path. On every other resolution path the file is bundled as a module instead: no asset is emitted and thehrefis left as written. Affected:Bun.build({ files })(the in-memory branch ofresolve_import_records),run_resolver, both its disk and in-memory branch),on_resolve,Successarm).fileloader for such references lives inline in the disk tail ofresolve_import_records(src/bundler/bundle_v2.rs, theimport_record_loaderblock). The three other places that pick the loader of an import record computeimport attribute, else extension loaderand stop there. The in-memory branch ofresolve_import_recordsis a copy of the disk tail that predates fix(bundler): copy non-JS/CSS files referenced as URL assets in HTML #27039 and has been drifting since (it also lacks the HTML import manifest handling that bundler: create the HTML import manifest on the plugin and in-memory resolution paths #38605 is adding to it).Fix
loader_for_import(importer_loader, kind, resolved_loader)is the one definition of the rule;resolve_import_records,run_resolver(both branches) andon_resolveapply it to the loader they computed. The plugin paths read the importer's loader back from the graph (loader_for_plugin_import); entry-point records have no importer and are passed through.resolve_import_recordsis deleted. A file map hit now takes the place of the resolver's result and goes through the same tail as a file on disk, so everything the tail does (this rule, the HTML import manifest, dedup through the path map and the resolve queue) applies to in-memory files too. The two things that were specific to that branch are kept:jsxcomes from the transpiler (a file map result has no tsconfig.json behind it), and the display path stays the map key. Everything else it set (known_target,tree_shaking,side_effects, theforce_node_envoverride) is what the tail sets anyway.test/bundler/bundler_files.test.ts: asset from an in-memory HTML file, from an HTML file on disk with the asset infiles, with a declining onResolve callback, and with a callback returning the path of an in-memory file; a server-side build importing an in-memory HTML file gets its manifest; jsx options and import attributes still apply to in-memory imports (guards for the removed branch). 5 of these fail on the current build, all pass with this one.test/bundler/bundler_html.test.ts:html/manifest-json-onresolve-declinesandhtml/manifest-json-onresolve-path(files on disk, resolved through a plugin), next to the existinghtml/manifest-json. Both fail on the current build.bundler_html,bundler_html_server,html-import-manifest,bundler_plugin,bundler_plugin_chain,bundler_loader,metafile,bun-build-api,bun-serve-html-manifest,bake/dev/plugins,bake/dev/html,css/doesnt_crashpass with the debug build;cargo clippy -p bun_bundleris clean.Background
with { type }) or the file's extension. Parsing loaders (json,toml,text, ...) turn the file into a JS module;Loader::should_copy_for_bundling()loaders (file,wasm, ...) register the file as an additional output that is copied to the output directory under a hashed name, and references to it are rewritten to that name. The HTML scanner records<script>as a statement import, stylesheets as@import-style imports, and everything else (manifest,img,video, ...) asImportKind::Url.resolve_import_records, which fills a resolve queue thatprocess_resolve_queueturns into parse tasks. A record that matches an onResolve filter is instead handed to the plugin and comes back later throughon_resolve: either with a path from the callback (Success), or, if every callback declined (NoMatch), throughrun_resolver, which runs the resolver itself. These two create the parse task directly.Bun.build({ files })puts its entries in a file map that is consulted before the resolver on each of these paths.Probe on the current build, and interplay with open PRs
Same three inputs (
page.htmlwith<link rel="manifest" href="./manifest.json">and<script src="./app.js">), outputs per resolution path on bun 1.4.0:With this branch all seven emit the asset and rewrite the href.
Open PRs touching the same lines, and what changes for whichever lands second:
run_resolver/on_resolve/process_resolve_queueparts are independent of this change; its additions to the removed branch are covered by the shared tail (the manifest test here is the bulk-path case of itsbundler_filestests).is_in_memorybranch at thepath_with_pretty_initializedcall with the unconditional call; this PR deliberately keeps the display path as it is today and does not assert on it.FileMap::resolvecarry the jsx options; on top of this change theresult.jsx = ...line at the top of the resolution loop becomes redundant.