Repository navigation
Conversation
…ts load
`check_browser_map` adds implicit extensions to the path it looks up. It
took their order from `Resolver.extension_order`, which follows the import
kind. `load_from_main_field` and `load_as_index_with_browser_remapping`
load the file with the order their caller passes, and that is the "main"
order for the "main" field and for the index of a package with "module".
So the probe could find the key of a file that the load does not pick.
With `"main": "./main"` and keys for `./main.mjs` and `./main.js`,
`import "p"` took the `./main.mjs` entry and `require("p")` took the
`./main.js` entry, although both load `main.js`.
`check_browser_map` now takes the order as an argument. The two entry
lookups pass the order of their load. The other four callers pass
`self.extension_order`, as before.
The lookup goes by name. It adds the implicit extensions in the order it is given, the first name that is a key wins, and it does not read the directory. The comment on `check_browser_map` and the test comments said more than that. New cases: a key for a file that is not on disk, a `./main.mjs` key whose target is missing, a `./main.js` key whose target is missing, a package that has only `main.mjs`, a `.mjs` key of "module" under `require`, and the two shapes where the "main" order gives another entry than esbuild. One `todo` case holds the shapes that still apply the key of a file that does not load. The cases now sit next to `BrowserMapMainFieldExtPermutations`.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Extension order and browser-map coverage src/resolver/resolver.rs, test/bundler/esbuild/packagejson.test.ts |
check_browser_map now matches candidates using the supplied extension order, which its callers pass for package-path, absolute-path, main-field, and index lookups. CSS main-field lookups use CSS extension order. Tests cover main-field and fallback probing, remap targets, import kinds, and CSS imports. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
b8801
-
Browser-map lookups now follow the load’s extension order, including for CSS. No merge-blocking regression was established.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the main change: browser-map lookups use the extension order of the load. Description check The description explains the problem, the fix, and how the author verified it. It does not use the template’s exact headings, but it provides the required information in detail. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- 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.
Comment @coderabbitai help to get the list of available commands.
|
Status: ready for review. Reproduced on linux x64 with a package Seven of the nine new cases in CI: build 133543 on b88011b finished with 180 of 181 jobs passed. The red job is one macOS x64 test shard. It fails in PR: #47034 |
|
Updated 4:03 AM PT - Oct 27th, 2026
❌ @robobun, your commit b88011b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 47034That installs a local version of the PR into your bun-47034 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/resolver/resolver.rs— pre-existing: browser-target builds fail withFile not found ""instead of getting an empty module when a package-path import resolves to a file that the package's own "browser" map sets to false. At src/resolver/resolver.rs:2396-2398 the code setsis_disabledon the old primary and then replaces primary withFs::Path::init_with_namespace(remap, b"file"), whoseis_disabledis false and whose text is the empty remap. Fix: build the disabled path the way src/resolver/resolver.rs:2139-2145 does, keeping the resolved text and settingis_disabled = trueon the path that is returned, soResult::path()yields None and the bundler emits an empty module. The base has the same lines; the PR's chain-map example now reaches them forimporttoo.Why this was flagged
A browser-target bundle contains
import "pkg/sub"and node_modules/pkg/package.json has"browser": {"./sub.js": false}with sub.js on disk. load_node_modules resolves the file as a plain file, and the post-resolution lookup at src/resolver/resolver.rs:2388-2393 returns an empty remap for it. src/resolver/resolver.rs:2396 setsis_disabledon the current primary, then src/resolver/resolver.rs:2397-2398 overwrites primary withFs::Path::init_with_namespace(remap, b"file"); src/paths/lib.rs:835 initialises that struct withis_disabled: false, and its text is "".Result::path()at src/resolver/result.rs:246-249 returns the primary because it is not disabled, so the bundler at src/bundler/bundle_v2.rs:7087 enqueues a parse of "" and reportsFile not found "". The early return at src/resolver/resolver.rs:2375 does not help because it runs before this lookup. The base branch has the same lines; the diff adds only the order argument at 2392, and by makingimporttake the./main.jskey it widens which imports reach this block.Verification: src/resolver/resolver.rs:2396 sets
result.path_pair.primary.is_disabled = true, then 2397-2398 overwrite primary withFs::Path::init_with_namespace(remap, b"file"), andinit_with_namespacein src/paths/lib.rs:830-838 constructsSelf { text, is_disabled: false, .. }, so the flag set on line 2396 is discarded. The base commit has the identical lines.
`load_as_file_or_directory` loaded the "main" field in the "main" order for every import kind. That order has no `.css`, so `@import "pkg"` did not find `main.css` for `"main": "./main"`. It loaded `main.js` when that file was there, and the build failed. The "browser" map lookup of "main" uses the order of the load since the previous commit. So a `./main.css` key stopped applying to a CSS import. With the CSS order for the load, the lookup finds the key again, and the package also resolves when it has no map. Tests: a CSS case for both packages, the observed output in the comment of the `todo` case, and no row that expects the key of a file that is not on disk.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The follow-up commits address all three points from my earlier review (CSS @ import of an extensionless "main" now keeps the Css order and has a test; the only-cjs-file row moved into the todo case; the todo comment now states the output Bun prints today), and this run found no bugs. A human look is still worthwhile because the change alters browser-target bundle output wherever browser-map keys differ between the import-kind order and the "main" order, and it pins a deliberate divergence from esbuild (keys for ./main.jsx / ./main.ts beside ./main.js) that the PR explicitly asks a maintainer to decide.
What was reviewed:
- All seven
check_browser_mapcallers: the two insideload_from_main_fieldandload_as_index_with_browser_remappingpass the sameextension_orderas theload_as_file/load_as_indexthat follows; the four top-level sites passself.extension_order, so they are behavior-preserving. - The
mf_ext_ordermatch at src/resolver/resolver.rs:5708 diverts onlyExtOrder::Css; every other variant still maps toMainField, and the same value feeds themodule->mainauto fallback, so CSS imports stay consistent on both paths. - The new tests exercise both
importandrequirefor each shape, with bundle-error expectations for the packages that must not resolve.
Extended reasoning...
The diff is 16 lines in src/resolver/resolver.rs plus 286 lines of itBundled tests: check_browser_map takes an explicit ExtOrder, the main-field and implicit-index lookups pass their load's order, and CSS imports keep ExtOrder::Css for "main". It touches no security-sensitive surface (no auth, crypto, or untrusted parsing beyond paths the resolver already handled). The code is small and internally consistent, no CODEOWNER covers resolver.rs, and the prior inline findings were addressed by later commits. Defer rather than approve because the change affects bundler output for browser builds across many package shapes and the PR itself leaves open a product decision about matching esbuild's .jsx/.ts-first order.
|
On the finding outside the diff ( Confirmed. I did not change that arm here, for two reasons.
The PR notes list the shape under "Not fixed here". |
Problem
bun build --target=browserapplies the"browser"entry of a file that"main"does not load.{"main":"./main","browser":{"./main.mjs":"./missing.js"}}besidemain.jsfails:error: Could not resolve: "p". Keys for./main.mjsand./main.jsgiveimportandrequiredifferent files.check_browser_map(src/resolver/resolver.rs) adds extensions in the import kind's order. The load uses the"main"order (.jsfirst, no.mjs).Fix
check_browser_maptakes the order as an argument.load_from_main_fieldandload_as_index_with_browser_remappingpass their load's order. The other four callers passself.extension_order."main"in the CSS order, so@import "pkg"findsmain.cssand its key.test/bundler/esbuild/packagejson.test.ts(9 new cases, 7 fail on main). Self-reviewed: 24 concerns raised, 23 addressed (Notes).Background
"browser"field maps a package file to another file or tofalse. The resolver looks an entry up before it loads it.Resolver.extension_orderaround the two lookups. A new caller would still get the wrong list silently.Downsides
--target=browser. 72 leave esbuild's output (a.ts,.tsxor.jsxkey, Notes). 25 stop resolving, as on 1.3.14.importof a chain map ("./main.js":"./b.js","./b.js":false) fails withFile not found "", likerequireon main."main", 5 without.Notes
Repro.
node_modules/phasmain.js. Runbun build entry.js --target=browser. The cells showimport "p", thenrequire("p").p"main":"./main","./main.mjs":"./missing.js"Could not resolve: "p""main":"./main","./main.mjs":falseimportisundefined"main":"./main","./main.mjs":"./a.js","./main.js":"./b.js""main":"./dist/index","module":"./dist/index.mjs","./dist/index.mjs":"./dist/browser.mjs",requireonly"dist/index.js"{"default":"dist/browser.mjs"}"dist/index.js""dist/index.js"Reach. #46985 lists this defect under "Not fixed here". I scanned the latest manifest of 17,148 widely used npm packages. 455 have a
"browser"map. In none of them does the entry key differ between the two orders. So no sampled package changes, in either direction.CSS imports.
load_as_file_or_directoryloaded"main"in the"main"order for a CSS import too. That order has no.css. So@import "pkg"with"main":"./main"andmain.cssfailed withCould not resolveon every target. Withmain.jsbeside it, the import loadedmain.jsand failed withCannot import a ".jsx" file into a CSS file. On 1.4.x the package resolved only through a./main.csskey, because the lookup used the CSS order. A CSS import now loads"main"in the CSS order, as esbuild does. The package resolves with and without a map, and the lookup follows the load.packagejson/BrowserMapMainFieldNoExtCSSImportcovers both.Not fixed here. The lookup does not read the directory. It can still pick the key of a file that does not load.
./main.cjsor./main.ctskey applies althoughmain.jsloads. Bun 1.3.14 and esbuild loadmain.js."main"loads its index in the order of the import kind. Its lookup uses that order. With keys for./index.mjsand./index.jsand onlyindex.json disk,importtakes the.mjsentry."./index.mjs":falsegives an empty module. A"main":"./index"line now changes that bundle. An extensionless"module"behaves the same way../main.cjsand./main.jsand onlymain.cjson disk, both statements take the./main.jsentry. esbuild does the same. Bun 1.3.14 took the./main.cjsentry.check_relative_pathand the lookup before resolution incheck_package_pathkeep the order of the import kind. With keys for./thing.mjsand./thing.js,import "./thing"andrequire("./thing")take different entries, as on 1.3.14. esbuild gives one. The load there has no single order: it is a file, an index, or a directory with its own"main"."browser"entry with no extension loads in the order of the import kind. That is the load, not this lookup."main"order can name a missing target. Then the package does not resolve, as in esbuild. Bun 1.4.x took the key of another extension.packagejson/BrowserMapMainFieldNoExtMissingTargetpins it.falseintoFile not found "". That is older than 1.3.14. bundler: port esbuild's "browser" map lookup and bundle a disabled path as an empty module #40832 removes that lookup.packagejson/BrowserMapEntryKeyOfMissingFile(todo) holds the first two shapes. A lookup by the name of the loaded file closes them. It is a larger change than this one and needs its own decision.Question for a maintainer. Take keys for
./main.jsxor./main.tsbeside./main.js. Bun loadsmain.jsfor"main", so this PR takes the./main.jsentry. esbuild loadsmain.jsxormain.tsand takes that entry.packagejson/BrowserMapMainFieldNoExtMainOrderpins the two rows. Say so if esbuild's output is the bar there.Matrix. 449 build cells, built with 1.3.14, main at 99ee03e, this PR and esbuild 0.25.1. 324 cells are generated. Their keys are one or two of the 9 implicit extensions of
"main":"./main". Each shape runs in a package and in a relative directory, forimportand forrequire. 125 cells are written by hand.--target=node: 3 cells change. They are CSS imports that now resolve.--target=browser: 219 cells change. 113 return to the 1.3.14 output. 36 more take esbuild's output. 70 are new for Bun.--target=nodeloads:--target=node. Their keys are.mjsor.mts.importandrequireof one package bundle different files in 45 of 220 pairs. With this PR, 7 do. The 7 have"module", orindex.mjsbesideindex.js."module"that one bundle imports and requires is now one copy, as in esbuild. Bun 1.3.14 and 1.4.x bundle two.node_modules:.ts,.tsxor.jsxwith.js,.cjsor.cts. Inside:.jsxwith.js,.cjsor.cts, and.tsor.tsxwith.cts.Order against other open PRs.
packagejson.test.ts(114 cases). With both, the chain rows printb.js.check_browser_mapand still readsself.extension_order. A merge has 5 conflict blocks inresolver.rs. Its new lookup inload_node_modulesmust then name an order. The compiler reports it.Measurements. Linux x64, debug and release builds of main at 99ee03e, without and with this change.
check_browser_mapoutside browser builds (gdb breakpoint counts): 0 and 0. That holds forbun file.js, for--target=node, and for a browser build of a package with no map.map.getsites):import, 44 to 42 forrequire.@importwith"main":"./main.css": 16 to 16.Bun.resolveSync("p"), package with"main": 1804 to 1813.load_as_file_or_directory, which now selects the order for"main".check_browser_map: 625 to 628 and 685 to 689 bytes.load_from_main_field: 1674 to 1699.load_as_index_with_browser_remapping: 1610 to 1683.load_as_file_or_directory: 2868 to 2855.MIMALLOC_SHOW_STATSprints nothing in these builds, andstraceis not installed.Suites run with the debug build.
test/bundler/esbuild/{packagejson,default,tsconfig,css,importstar,extra,loader}.test.ts,test/bundler/bundler_{browser,npm,cjs,regressions,edgecase,html}.test.ts,test/bundler/css/css-modules.test.ts,test/js/bun/css/css-loader.test.ts,test/js/bun/resolve/{resolve,resolve-error}.test.ts,test/bake/dev/esm.test.ts. Two cases exceed their time limit under the debug build, with and without this change. Both pass on release builds:default/TopLevelAwaitForbiddenRequireSourceMapCLIandedgecase/DeepImportDiamondDAG.Self-review. One concern is open: no passing case has a key whose file is missing and that wins. A row for it (
only-cjs-file) was removed in review, because thetodocase expects the opposite rule. Three suggestions are not taken.todocase holds them.