Skip to content

bundler: resolve files imports through the shared steps (HTML import manifest, url assets) - #38605

Open
robobun wants to merge 14 commits into
mainfrom
farm/30313fcf/html-import-plugin-resolve
Open

robobun wants to merge 14 commits into
mainfrom
farm/30313fcf/html-import-plugin-resolve

Conversation

@robobun

@robobun robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Server code that imports an HTML page from Bun.build({ files }) gets no manifest. The output keeps import page from "/app/page.html" and exits with Cannot find module '/app/page.html', or serves the page on disk.
  • resolve_import_records has its own branch for files (src/bundler/bundle_v2.rs:6900 on main). It copies the shared steps without the manifest rule (:7301) and the url asset rule (:7283).

Fix

Background

  • files is the in-memory file map of Bun.build.
  • HTML import manifest: the bundler builds the imported page for the browser. A JSON list of the output files replaces the import. Bun.serve reads it.
  • The first version also patched run_resolver and on_resolve: that broke plugins that load .html as JS. A patch inside the files branch keeps the copy.

Downsides

Notes

Merge order

  1. bundler: keep the import kind of server-side HTML imports so import() and require() of the manifest link #38621, then this PR. The shared steps rewrite the record kind of a page import. await import() of a page then fails at run time with ReferenceError: require_<name> is not defined. A page on disk does that on main. An in-memory page does it with this PR. bundler: keep the import kind of server-side HTML imports so import() and require() of the manifest link #38621 removes the rewrite. It merges into this branch with no conflict. On a debug build of that merge, await import() and require() of an in-memory page return the manifest.
  2. This PR, then bundler: resolve ./ and ../ keys of Bun.build files against the cwd in FileMap::put #46667. bundler: resolve ./ and ../ keys of Bun.build files against the cwd in FileMap::put #46667 makes a ./ key an absolute path. Without the manifest, server code that imports such a page exits at startup.
  3. This PR, then bundler: resume the shared resolve pass for imports that went to onResolve plugins #45408. bundler: resume the shared resolve pass for imports that went to onResolve plugins #45408 takes the onResolve routes. Two tests for an in-memory page behind an onResolve plugin left this PR. A comment on bundler: resume the shared resolve pass for imports that went to onResolve plugins #45408 has them.

What changes for an import that files answers

Baselines: bun 1.4.2 and a release build of main (179664e). Both give the same result in each case below.

  • A server-side import of an HTML page gets the manifest. The page is built for the browser. Before, by key shape:
    • An absolute key with no file on disk: the bundle exits 1 with Cannot find module '<dir>/page.html'. A compiled executable exits 1 with Cannot find module '<dir>/page.html' from '/$bunfs/root/app'. With this PR the executable answers 200 for the page, its script and its stylesheet.
    • A key that overrides a page on disk: the bundle serves the page on disk, not the text from files.
    • A ./ key: the bundle keeps import page from "./index.html" and loads the emitted page at run time. It starts from any directory.
  • Bun.serve looks for the files of a manifest in the working directory. A bundle with an in-memory page started elsewhere now fails with TypeError: Bundled file "./chunk-<hash>.js" not found. A page on disk does the same on main. Bun.serve: find the files of a built HTML import from the server bundle's folder #46019 wanted to lift that rule and is closed.
  • await import("./index.html") of an in-memory page throws ReferenceError: require__ is not defined. require() of it returns { default: manifest }. On 1.4.2 both ran for a ./ key.
  • A url asset of a page (<link rel="manifest" href="./manifest.json">) is copied when the page or the asset is in files. Before, the href stayed as written and no file was written. A standalone HTML build (compile: true, target: "browser") now inlines the asset as a data: URL.
  • A script of that page that also imports the linked file gets the asset URL. Before, it got the parsed JSON. A page on disk gives the URL on 1.4.2 and on main. bundler: key modules by (path, loader) so one file under two import attributes is two modules #41834 keys a module by path and loader.

What does not change

  • The display path of an in-memory file is its key. Its jsx options are those of the build, not those of a tsconfig.json beside a file that the key overrides. Two guard tests pin this. bundler: give files imports the same display path as disk files #38620 and bundler: apply the jsx option to entry points that come from files: #38609 are about these two rules.
  • An import attribute or a loader entry decides how an in-memory .html file is loaded. Three guard tests pin with { type: "text" }, with { type: "file" } and loader: { ".html": "text" }.
  • A build with an onResolve plugin. Seven plugin setups give the same outputs on main and on this branch. A plugin that declines the import still fails the build with No matching export in "page.html" for import "default".

Limits

Why the onResolve routes left this PR

The first version made the manifest in run_resolver and in on_resolve from the .html extension, before an onLoad plugin ran. The setup below is a page on disk and target: "bun". An onLoad plugin returns { contents: 'export default "template of the plugin"', loader: "js" }.

Setup 1.4.2 and main First version
onResolve declines, onLoad loads as JS the string of the plugin { files } with no index, and a stray page.js
onResolve answers in its own namespace, onLoad loads as JS the string of the plugin the same
onResolve answers with the file path, onLoad loads as JS the string of the plugin the same

#45408 sends a declined or answered import back through resolve_import_records and chooses after onLoad.

Self-review: the four concerns that stay

Measurements (release builds of main 179664e and of this branch, linux x64, one CPU, perf_event_open user instructions)

  • No files, 2,001 modules and 22,000 import records on disk, 31 interleaved runs. The median is 548.03 M on main and 548.21 M here. The quartile ranges overlap (547.90 to 548.21 M, 547.95 to 548.41 M). With 42,000 import records the medians are 949.88 M and 950.01 M, and the ranges overlap.
  • The same project in files, 21 interleaved runs: median 489.22 M on main and 489.70 M here. That is 22 instructions for each import record that files answers. No new allocation: read from the code.
  • One detail decides the first number. The resolve loop now has two break sites. A second binding of the result then costs one more copy of 296 bytes for each import record (548.87 M). The result now has one binding.
  • Binary: the stripped file has the same size. .text is 82,164,617 bytes on main and 82,165,641 here. The sizes of the four changed functions sum to -90 bytes (llvm-nm -S).

Verification

  • test/bundler/bundler_files.test.ts: 42 pass on a debug build and on a release build of this branch. On a release build of main 9 fail and 33 pass. The 5 other new tests are guards.
  • test/bundler/html-import-manifest.test.ts is not in the diff. The merge with main had left a second readManifests in it that shadowed the helper of main.
  • Debug build: bundler_html, bundler_html_server, bundler_plugin, bundler_plugin_chain, bundler_barrel, bundler_loader, bundler_bun, standalone, metafile, esbuild/loader, css/doesnt_crash, bun-serve-html, bun-serve-html-manifest, bake/dev/html, bake/dev/plugins.
  • Release build of this branch: bundler_edgecase (188 pass), bun-build-api (89 pass), cli/check/entry-points (473 pass). The local debug build hit test timeouts in these three files. A debug build of an earlier head of this branch hit the same timeouts.
  • cargo clippy -p bun_bundler. macOS and Windows were not run locally.

Related


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_files.test.ts

…resolution paths

A server-side import of an HTML file only became a manifest module plus a
browser entry point when the import record was resolved by the bulk pass in
resolve_import_records / process_resolve_queue. Records resolved one at a
time (an onResolve plugin declining the import, an onResolve plugin returning
the HTML file's path) and imports of in-memory `files:` entries bundled the
HTML as an ordinary module of the server graph instead, so the import either
failed with 'No matching export in "page.html" for import "default"' or, for
require() and in-memory files, built into output that never had a manifest.

Factor the decision into is_server_html_import, make
generate_server_html_module return the manifest module's index, and add
enqueue_server_html_import for the one-at-a-time paths: it creates the
manifest module in the importing graph and parses the HTML file as a browser
entry point unless the browser graph already has it.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between d3663aa and 3697c11.

Files selected for processing: 2
  • src/bundler/bundle_v2.rs
  • test/bundler/bundler_files.test.ts

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 615748e and d3663aa.

Files selected for processing: 1
  • test/bundler/bundler_files.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

FileMap imports now use shared resolution and loader handling. In-memory parse tasks use JSX options from the selected transpiler. New tests cover HTML build outputs, manifests, server responses, loader selection, and referenced assets.

Changes

In-memory Imports

Layer / File(s) Summary
Shared FileMap import resolution
src/bundler/bundle_v2.rs, test/bundler/bundler_files.test.ts
FileMap imports now pass through the shared resolution loop. In-memory paths copy their pretty value from their text, and parse tasks use JSX options from the selected transpiler. Tests cover JSX options and text-loader selection.
HTML build output and manifest handling
test/bundler/bundler_files.test.ts
Tests cover HTML manifests and generated assets, imports from disk entry points, repeated and distinct pages, loader-based HTML handling, and referenced JSON assets.
Serving generated pages
test/bundler/bundler_files.test.ts
Tests build and run servers for Bun and Node targets and check responses for generated pages, scripts, and stylesheets.

Suggested reviewers: jarred-sumner

Priority: Normal

Merge Risk: Minimal · previous review d3663

No concrete merge-blocking risk is established by the reviewed changes.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the main change: routing files imports through shared resolution steps for HTML manifests and URL assets.
Description check Passed The description explains the problem, fix, scope, limitations, and verification results. It provides the information required by the repository template, although it uses custom headings instead of th…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, fix, scope, limitations, and verification results. It provides the information required by the repository template, although it uses custom headings instead of the exact template headings.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: the branch has main merged in and now covers only the files route (615748e). CI is running.

Reproduced on bun 1.4.2 and on a release build of main (179664e). The build is target: "bun", server.js imports ./page.html, and the page is in Bun.build({ files }). The build reports success. The output keeps import page from "<dir>/page.html" with { type: "html" }, and bun dist/server.js exits 1 with Cannot find module '<dir>/page.html'. test/bundler/bundler_files.test.ts has 9 tests that fail there and pass on this branch.

The onResolve routes that this PR covered before are the subject of #45408.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

I was working on the files: branch of resolve_import_records for the same symptom (an in-memory .html import in a target: "bun" build keeps the raw import, and the page's scripts get bundled under the server target). This PR covers that, so I am not opening a separate one.

One thing that may be worth folding in here, or doing as a follow-up: the files: branch is a copy of the tail of the disk branch that has drifted, and the manifest lines are not the only thing it is missing. It also skips the loader override for url assets referenced from an HTML file, so an in-memory page with <link rel="manifest" href="./manifest.json"> bundles manifest.json with the json loader: no asset is emitted and the href is left as-is. On disk the file is copied to the output and the href rewritten.

Branch with an alternative shape of the resolve_import_records part: main...farm/7525473c/files-html-import-manifest (ce68741). A files: hit becomes the resolve_result and falls through the shared tail; the only virtual-file specifics kept are jsx from the transpiler and pretty being the map key. That removes the duplicated branch (net -49 lines) and fixes both the manifest case and the asset case. Tests for the manifest (bulk path, importer on disk, the same page imported twice) and for the asset are in bundler_files.test.ts there. It conflicts textually with this PR in bundle_v2.rs and bundler_files.test.ts, so whichever lands second needs a rebase; the run_resolver / on_resolve parts of this PR are not affected either way.

Comment thread src/bundler/bundle_v2.rs Resolved
@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up from #38621, which fixes the import("./page.html") / require("./page.html") case this PR's description lists as found along the way: the cause of that one is the import_record.kind = ImportKind::HtmlManifest assignment itself. The linker wraps a module (or, with splitting, gives it a chunk of its own) based on the Require / Dynamic kind of the records that import it, and the printer emits the import from the same kind, so once the kind is replaced the manifest module is never wrapped while the call to require_page() is still printed. #38621 removes the assignment on the bulk path and uses the is_html_entrypoint predicate for known_target directly (the only thing that read the rewritten kind).

This branch adds the same assignment to run_resolver, the on_resolve Success arm and the files: branch, which would carry that bug onto those paths. Whichever of the two lands second should drop the assignments; the manifest wiring (server_source_indices plus the browser graph's path map entry) does not depend on the kind. The metafile then reports the real kind (import-statement, dynamic-import, require-call) on every path, so the byte-identical comparison in this PR's description still holds, just with those labels.

…disk

Bun.build({ files }) resolved an import that hits the in-memory file map in
a separate branch of resolve_import_records that duplicated the tail of the
disk resolution and had drifted from it. Two things it did not do:

- importing an .html file into a server build did not turn the import into
  an HTML import manifest: the output kept a literal import of the virtual
  path and the page's scripts were bundled for the server target
- a url asset referenced from an in-memory .html file (for example
  <link rel="manifest" href="./manifest.json">) kept its parsed loader
  instead of being copied to the output as a file

A file map hit now only substitutes for the resolver's result and then goes
through the shared steps, keeping the two things that are specific to
virtual files: the jsx options come from the transpiler (there is no
tsconfig.json to consult) and the display path is the map key as-is.
@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:12 AM PT - Oct 27th, 2026

✅ @robobun, your commit 3697c110c604b7eac91364ede03cbc497cbc1959 passed in Build #133525! 🎉


🧪   To try this PR locally:

bunx bun-pr 38605

That installs a local version of the PR into your bun-38605 executable, so you can run:

bun-38605 --bun

Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
…side generate_server_html_module

The two call sites that reserve their map slot with get_or_put now fill it
the same way their non-HTML siblings do, and the two that use get-then-put
call put; generate_server_html_module only creates the module.
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

#38635 now carries the restructure mentioned above (the files: branch of resolve_import_records is removed and a file map hit goes through the disk tail), as the fix for the url asset loader gap. It does not touch the run_resolver / on_resolve / process_resolve_queue parts of this PR; whichever of the two lands second only needs to drop or re-apply the hunk in that branch and re-merge the tests appended to bundler_files.test.ts.

Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
…nifest module

The linker wraps a module (or gives it its own chunk) based on the Require /
Dynamic kind of the records importing it, and the printer emits the import
from the same kind, so replacing the kind with HtmlManifest is what breaks
import() and require() of an HTML file. The manifest wiring does not depend
on it: the module is found through html_imports.server_source_indices and the
browser graph's path map. Do not introduce the rewrite on the paths added
here; the existing one on the bulk path is removed separately.
@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Re the files: branch: agreed that replacing the drifted copy is the better shape, so that change is folded in here as 70b34c6 (same commit, conflicts resolved against this branch), together with its tests; the two plugin-path cases for in-memory pages were added to the same describe block, and the three-line patch to the old branch is gone with the branch.

Re #38621: thanks, that matches what I saw when probing (import() of the page printed a call to a require_page that was never defined, on the bulk path and, with the rewrite copied over, on the plugin paths too). aa0a705 drops the kind assignment from the paths this PR adds, so the two changes compose in either landing order: this PR never introduces the rewrite, #38621 removes the existing one. With that commit import() of an HTML file already works on the plugin paths, and the outputs of the plugin and plugin-less builds are still byte-identical; until #38621 lands the metafiles differ only in the kind label. The description is updated accordingly.

@robobun

robobun commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator Author

#45408 covers the onResolve half of this PR: a declined or answered import of an HTML file goes through resolve_import_records, so a server build gets the manifest. The onResolve tests of this PR are in #45408. The files half of this PR is not covered.

Conflicts:
- src/bundler/bundle_v2.rs, on_resolve: the non-HTML branch keeps the
  requested_file_loader call and the is_entry_point field from main.
- src/bundler/bundle_v2.rs, resolve_import_records: the separate branch
  for in-memory files stays removed. The shared steps already use
  forced_jsx_development from main.
- test/bundler/bundler_files.test.ts: keeps the tests of both sides.
…odule

resolve_import_records cloned the jsx options of the build for each
import record that the `files` map answers. A record whose module is
already in the graph or in the resolve queue does not need them. The
clone now happens where the parse task of a new module gets its jsx
options, as it did before the in-memory branch was merged into the
shared steps.
Without a manifest the bundle keeps an import of the page's path. No
file is at that path, so the bundle exits at startup with "Cannot find
module". The test builds to an outdir and runs the output.
The run_resolver and on_resolve hunks chose manifest or module from the
`.html` extension before an onLoad plugin ran. A plugin that takes an
.html import in onResolve and loads it as JS then got a manifest with no
`index`. bun 1.4.2 gives that import the module of the plugin. Those
routes need the choice after onLoad, so they leave this branch.

What stays is the stand-in in resolve_import_records: an import that
the `files` map answers takes the shared steps. is_server_html_import,
enqueue_server_html_import and the changed signature of
generate_server_html_module have no caller left that needs them. Main's
text is back there.

Tests: html-import-manifest.test.ts is main's file again. The merge had
left a second readManifests in it that shadowed main's helper. The two
plugin tests of bundler_files.test.ts leave with the hunks. New tests:
Bun.serve serves the page from a bundle for bun and for node, the
`input` of the manifest, import attributes and the `loader` option on
an in-memory page, a page on disk with an in-memory url asset, and a
standalone HTML build.
With two `break` sites in the resolve loop, the second binding
`let mut resolve_result = resolve_result;` became a real copy of the
296-byte result for each import record. One mutable binding takes the
value from either site directly. The `files` lookup moves into the loop
so that no optional result lives across it.

Release builds, 2,001 modules and 22,000 import records, no `files`,
medians of 31 interleaved runs: 548.03 M instructions on main, 548.87 M
with the second binding, 548.21 M with this commit. The quartile ranges
of main and of this commit overlap.
A `files` key keeps "/" separators on Windows. The root has "\\"
there, so the path code does not make the key relative and `input` is
the whole key. The exact value stays pinned on the other platforms.
@robobun robobun changed the title bundler: create the HTML import manifest on the plugin and in-memory resolution paths bundler: resolve files imports through the shared steps (HTML import manifest, url assets) Oct 25, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, two refactor concerns were checked and ruled out: dropping the old branch's explicit side_effects = HasSideEffects is behavior-preserving because bun_ast::SideEffects defaults to HasSideEffects and ParseTask::init copies the FileMap result's default primary_side_effects_data (src/bundler/bundle_v2.rs:1068-1077, 3106); and the last_error early continue at src/bundler/bundle_v2.rs:7107 now skipping in-memory source-index binding for later records is not observable, since a set last_error clears the resolve queue and the build aborts before link (src/bundler/bundle_v2.rs:6580-6606).

Extended reasoning...

The change folds the Bun.build files in-memory resolution branch in src/bundler/bundle_v2.rs into the shared post-resolve path and adds tests in test/bundler/bundler_files.test.ts; it touches no auth, crypto, or injection surface. Five confirmed findings are posted inline, so approval is not on the table; this note only records two additional refactor concerns that were verified as not affecting behavior.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 src/bundler/bundle_v2.rs — Users who await import() an in-memory HTML page under a ./ key in a target: "bun" build now get a bundle that throws ReferenceError: require__ is not defined at run time, where the base build ran. At src/bundler/bundle_v2.rs:7263 the shared path now rewrites every in-memory server-side HTML record's kind to ImportKind::HtmlManifest, including dynamic import records, so the printer loses the dynamic-import shape. Fix: do not rewrite (or print a manifest-returning promise for) DynamicImport and Require records until the printer handles HtmlManifest for those kinds, covering disk and in-memory pages alike; the PR notes this depends on #38621 landing first, and that note holds only if it merges in that order.

    Why this was flagged

    Trigger: Bun.build({ target: "bun", files: { "./page.html": ..., "/app/server.js": "const page = await import('./page.html')" } }). FileMap::resolve at src/bundler/bundle_v2.rs:989 matches the ./page.html key directly, so at 6911-6912 is_in_memory = true and the record takes the shared path. At 7248-7250 is_html_entrypoint is true and 7263 overwrites import_record.kind from DynamicImport to ImportKind::HtmlManifest; 7321-7323 then generates the manifest module. On the base branch the old early block kept the record kind and just parsed the file, so the emitted bundle kept import("./page.html") and loaded the emitted page at run time. After the change the printed dynamic import references an undefined wrapper and the process throws ReferenceError: require__ is not defined. The PR description states this itself under Downsides and Merge order (#38621 first); nothing in this checkout enforces that order, so merged as-is it is a run-time crash for a previously working input.

    Verification: The PR description's "Downsides" section states exactly this and the "Merge order" note asks for #38621 to land first, but nothing in this checkout enforces that order and no test pins the dynamic-import case. At src/bundler/bundle_v2.rs:7263 import_record.kind = ImportKind::HtmlManifest; is applied regardless of whether the record's kind was Stmt or Dynamic.

  • 🔴 src/bundler/bundle_v2.rs — Users whose in-memory page both links a JSON file and imports it from a script now get the asset URL string in the script instead of the parsed JSON. The url-asset override at src/bundler/bundle_v2.rs:7235-7242 now runs for files hits and forces ./manifest.json to Loader::File; the later script import then hits the path-only path_to_source_index_map at src/bundler/bundle_v2.rs:7252 and is bound to that File module. Fix: key the per-target module lookup by path and loader (or create a separate module when the requested loader differs), so a <link> asset and a JS import of the same files key each get their own module; on the base branch the in-memory script received the JSON object. [also at: src/bundler/bundle_v2.rs:6912 - Users of Bun.build({ files }) whose page links a .json file that its script also imports now get the asset URL string in the script instead of the parsed object.]

    Why this was flagged

    Trigger: Bun.build({ entrypoints: ["/app/index.html"], files: { "/app/index.html": '<link rel="manifest" href="./manifest.json"><script src="./app.js"></script>', "/app/manifest.json": '{"name":"app"}', "/app/app.js": 'import m from "./manifest.json"; console.log(m.name)' } }). The HTML source is resolved first; its url record for ./manifest.json now comes from the FileMap inside the shared loop (src/bundler/bundle_v2.rs:6904-6912) and reaches the override at src/bundler/bundle_v2.rs:7235-7242, which sets import_record_loader = Loader::File because the importer loader is Html, kind is Url and Json does not should_copy_for_bundling(). When app.js is resolved, its import hits self.path_to_source_index_map(target).get(path.text) at src/bundler/bundle_v2.rs:7252 and gets source_index of the File module, whose default export is the asset URL string (src/bundler/ParseTask.rs:1377-1445). On the base branch the FileMap branch exited before the override, so manifest.json was a Json module and m.name was "app"; after merge m.name is undefined with no build error.

    Verification: In /home/claude/bun/src/bundler/bundle_v2.rs, line 7252 if let Some(id) = self.path_to_source_index_map(target).get(path.text) finds the File module and binds import_record.source_index = Index::init(id) (7257); the loader is not part of the key, so the script receives the asset URL string, and m.name is undefined. The new tests at test/bundler/bundler_files.test.ts:990-1029 cover only the asset-copy side.

  • 🟡 src/bundler/bundle_v2.rs — Users who bundle a ./-keyed in-memory HTML page into a server build and start the bundle from outside the outdir now exit at startup. src/bundler/bundle_v2.rs:7248 now turns the in-memory page into a manifest, and Bun.serve checks each manifest path against the working directory (src/runtime/server/server_body.rs:719), failing with Bundled file "./chunk-<hash>.js" not found. On the base the import stayed import page from "./index.html", which Bun resolved next to the bundle from any directory. The PR lists this as a downside; it follows from the stated purpose. Fix: keep such bundles startable from any directory, e.g. resolve manifest paths against the bundle's own directory instead of the cwd, which also covers pages on disk.

    Why this was flagged

    Trigger: Bun.build({ target: "bun", outdir: "dist", files: { "./index.html": ..., "./server.js": 'import page from "./index.html"; Bun.serve({ routes: { "/": page } })' } }), then bun dist/server.js from the project root. On this branch is_html_entrypoint at src/bundler/bundle_v2.rs:7248-7250 is true because the loader is Html and the target is server-side, so the record becomes ImportKind::HtmlManifest (7263) and generate_server_html_module (7322) replaces the import with a manifest whose path entries are ./index-<hash>.html, ./chunk-<hash>.js. At run time from_options probes each path with sys::exists_at_type(sys::Fd::cwd(), zpath) (src/runtime/server/server_body.rs:719), throwing Bundled file ... not found (server_body.rs:729) when the cwd is not the outdir. On the base the old in-memory branch created a plain Html ParseTask, the output kept import page from "./index.html", and Bun resolved that relative to the bundle file, so the server started from any directory. No safeguard applies: the PR's test runs the bundle with cwd: dist only.

    Verification: Trigger: a target: "bun" build with an in-memory .html page under a ./ key whose bundle is started with a cwd other than the outdir. The record becomes ImportKind::HtmlManifest; from_options checks sys::exists_at_type(sys::Fd::cwd(), zpath) (src/runtime/server/server_body.rs:719), throwing Bundled file "..." not found. On the base the server output keeps import page from "./index.html".

Comment thread test/bundler/bundler_files.test.ts Resolved
Comment thread src/bundler/bundle_v2.rs Resolved
On Windows the key keeps "/" separators and the root has "\\". The
path code finds no common root for the two forms, so `input` is the
whole key. The test now expects that value instead of a suffix match.
@robobun

robobun commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Replies to the review findings:

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked the three behaviors the removed FileMap block used to set explicitly: side_effects is unchanged because SideEffects::HasSideEffects is the Default that ParseTask::init picks up from the FileMap result (src/ast/loader.rs:255); the external branch at src/bundler/bundle_v2.rs:7125 cannot fire because result_for_key leaves flags at default; and the dev-server branch at :7144 is unreachable for in-memory hits because file_map is only set from the Bun.build completion task (src/runtime/api/js_bundle_completion_task.rs:1338), which has no dev server.

Extended reasoning...

The diff folds the in-memory FileMap lookup into the shared resolve loop in src/bundler/bundle_v2.rs so virtual files take the same post-resolution steps as disk files (HTML manifest, URL-asset loader forcing, browser target for manifests), and adds ~330 lines of tests in test/bundler/bundler_files.test.ts. It touches no security-sensitive surface. One confirmed nit (relative-key manifest input) is posted inline; the ruled-out items above are the refactor's behavior-preservation questions that I verified from the code this run.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 src/bundler/bundle_v2.rs — nit: Users who import an in-memory HTML page under a relative files key such as ./index.html into a target: "bun" build now get a manifest whose input reads ../../.././index.html. The in-memory branch at src/bundler/bundle_v2.rs:7322 hands the raw key to generate_server_html_module as the page's source path, and src/bundler/HTMLImportManifest.rs:411 relativizes it against root_dir as if it were absolute. Fix: for an in-memory source emit the key itself as input unless it is absolute and under root_dir, at both HTMLImportManifest.rs:274 and :332. The PR's Limits note calls this known; it holds, but input is typed as the original source path.

    Why this was flagged

    Trigger: Bun.build({ entrypoints: ["./server.js"], target: "bun", files: { "./server.js": 'import page from "./index.html"', "./index.html": "..." } }), the Bun.build JS API with a relative files key. src/bundler/bundle_v2.rs:7248-7264 and :7321-7323 now mark the in-memory import as ImportKind::HtmlManifest and create the server module with Source.path.text set to the key ./index.html. When the manifest is written, src/bundler/HTMLImportManifest.rs:274-278 calls source_path_relative_to_root(root_dir, "./index.html"); root_dir is the cwd. longest_common_path_generic at src/paths/resolve_path.rs:335-337 returns "/" for a leading-dot path, and relative_to_common_path then prepends one .. per cwd segment, giving ../../.././index.html. On the base no manifest was produced for a files import at all, so there was no input to be wrong. Bun.serve itself does not read input (src/runtime/server/server_body.rs:669-695), so the harm is a wrong public field, not a failed route. The author's Limits note names this and points at #46667 for ./ keys; nothing in this PR guards it.

    Verification: The PR description's "Limits" section states "A key that is not an absolute path gives a wrong manifest input, for example ../../.././index.html." src/bundler/HTMLImportManifest.rs:271-279 computes input via source_path_relative_to_root(root_dir, sources[...].path.text), giving ../../.././index.html. Bun.serve is unaffected. On the base branch this case produced no manifest at all.

@robobun

robobun commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

On the relative key nit: the wrong input comes from the key itself. A ./index.html key is not an absolute path, so every step that treats a source path as absolute gets it wrong (the display path, the manifest input, the lookup of the same file under two spellings). #46667 makes such a key absolute in FileMap::put, where every consumer sees the fixed form. A special case for in-memory sources in HTMLImportManifest.rs would fix only the input field, so I leave this to #46667.

@coderabbitai coderabbitai Bot mentioned this pull request Oct 25, 2026
5 tasks done
robobun added a commit that referenced this pull request Oct 25, 2026
… into robobun/40800912/files-relative-keys

This branch is now based on #38605 at d3663aa. The two conflicts are in
text that both branches have: the hunk of #38605 in resolve_import_records,
and its tests. The result keeps the two places where this branch differs
(the fourth argument of FileMap::resolve, and the display path of a ./ key).
@robobun

robobun commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

A note from #46667, which is now stacked on this branch (it contains d3663aa).

The entry point is the sibling site of this fix. A files hit for an entry point still has its own branch in enqueue_entry_points_normal (src/bundler/bundle_v2.rs), and that branch does not take the shared steps either.

Repro on this branch at d3663aa, with src/page.tsx on disk and no react installed:

import { resolve } from "node:path";
const page = resolve("src/page.tsx");
await Bun.build({
  entrypoints: [page],
  files: { [page]: "console.log(<div>page from memory</div>);" },
  jsx: { runtime: "classic", factory: "myFactory", fragment: "MyFragment" },
});

It fails with Could not resolve: "react/jsx-dev-runtime". The entry point gets the default JSX options, not those of the build. Bun 1.4.3 does the same. The same file as an import gets myFactory with this PR.

#38609 has tests for the jsx option of such an entry point, and #38594 covers an in-memory HTML entry point of a server build (its script has the // @bun pragma).

Why it matters for #46667: for this reason it sends an entry point that is the absolute path of a ./ key to the disk resolver, and not to files. If the entry point takes the shared steps here, #46667 can drop that rule.

@robobun

robobun commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main (8a0e49e) into this branch.

  • One conflict, in resolve_import_records in src/bundler/bundle_v2.rs. This PR deletes the separate FileMap branch: a files hit sets is_in_memory and takes the shared steps. Main (98a4365, Fix use-after-free of BuildMessage.position.file: real lifetimes for Source, Path, ImportRecord and resolve results #46834) moved that branch to the real lifetimes. The resolution keeps the code of this PR, and the binding is let mut resolve_result: _resolver::Result<'a>.
  • One more edit in the same function: path.pretty = self.graph.heap.alloc_slice_copy(path.text); replaces the unsafe detach_lifetime form of this PR. Main uses that form for the same case. It is the same arena and the same bytes.
  • cargo check --workspace passes on the merged tree (Linux x64).
  • Test, on a local debug build that contains this merge and the main merges of five other open PRs, with --timeout 30000: test/bundler/bundler_files.test.ts (42 pass).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant