[robobun]

Problem

• With --target=browser, require() of a path that a browser map sets to false returns a function: (() => ({})) (print_disabled_import, src/js_printer/lib.rs).
• require("pkg/foo/bar") with the key "./foo/bar" bundles the Node file or fails with Could not resolve. load_node_modules (src/resolver/resolver.rs) reads the map after the file lookup.
• From pkg/sub/foo.js, require("sub") with the key "./sub" resolves to foo.js itself.

Fix

• check_browser_map is a port of esbuild's checkBrowserMap. load_node_modules calls it for each package candidate.
• A JavaScript import of a disabled path loads one empty module, (disabled):<path>, as in esbuild.
• Main's lookup after resolution stays. esbuild has none.
• Verified: test/bundler/esbuild/packagejson.test.ts, test/bundler/{bundler_browser,metafile}.test.ts. 59 of 104 new cases fail on main. Self-reviewed: 24 concerns raised, 16 addressed, 8 in Notes.

Background

• The browser field maps a file or a bare specifier to a file, a package, or false (disabled).
• The bundler keys a module by its path. One bundle can hold a disabled file and the file itself, so Path.is_disabled_module gives the empty module its own key.
• @dylan-conway asked for esbuild's shape. A printer-only {} (bundler: emit an empty object for modules disabled via the browser field #33059) gives each require a new object.

Downsides

• Breaking: a call of a disabled module throws. A browser bundle of iconv-lite 0.4 fails under Node.js or Bun.
• DevServer: land dev server: declare the namespace of an import that the browser target disables #46579 first. Without it, an ESM import of a path that only this PR disables has no binding there (ReferenceError).
• The key "./x.js" no longer matches require("x") by name. Binary: +9,984 bytes of text.

Notes

Repro (main, bun build e.js --target=browser):

// node_modules/pkg/package.json: {"main":"index.js","browser":{"./node.js":false,"nodeonly":false}}
// node_modules/pkg/index.js: var n = require("./node.js"), m = require("nodeonly"); module.exports = [typeof n, Object.keys(n).length, typeof m];
["function",0,"function"]   now ["object",0,"object"]

// node_modules/pkg/package.json: {"browser":{"./foo/bar":"./file"}}   (foo/bar.js absent)
require("pkg/foo/bar")   ->  error: Could not resolve   now bundles file.js

// node_modules/pkg/package.json: {"browser":{"./sub":"./sub/foo.js","./sub/sub":"./sub/bar.js"}}
// node_modules/pkg/sub/foo.js: require("sub")   ->  require_foo() (itself)   now sub/bar.js

// node_modules/@s/p/package.json: {"browser":{"./x.js":"./a.js"}}
import "@s/p/x.js"   ->  x.js   now a.js   (the same import of an unscoped package gave a.js)

History of this PR. There are three revisions.

• Revision 1 removed main's lookup after resolution, to match esbuild. A review measured the cost on real packages: require("typeorm") (0.3.20) stopped building (Browser build cannot require() Node.js builtin: "dns"), and yaml@1.10.2 silently switched to its Node build.
• Revision 2 (a17bfcb) was rebuilt on main and kept that lookup. A second review found defects in it. The queue of one importer keyed the empty module by its display path, so one input gave one of two bundles at random. ./dir and ./dir/ were two modules. export * from <disabled> became a build error (@aws-sdk/token-providers@3.664.0 stopped building). The package_json condition of the lookup after resolution was gone. It also carried resolver: do not look up a "browser" map target again after resolution #46985 whole.
• This revision fixes each of those. A third review then found more: a main with a trailing separator was a second module, a disabled .mjs module entry with a live main gave one of two bundles, and the body had wrong claims. Those are fixed too. The next sections describe the result.

The lookups for a package subpath (load_package_file_with_browser_map, called for each package candidate of the node_modules walk and of NODE_PATH):

1. A key that names the path as the import spells it (./foo/bar or foo/bar for pkg/foo/bar, or its index) wins over the file, as in esbuild.
2. Else the file loads, and a key that names the loaded file applies to it (./foo/bar.js).
3. Else a directory loads through its own main and index lookups. That is main's code.
4. Else a key with an implicit extension names a file that does not exist, and its target loads.

esbuild does one lookup with implicit extensions before the file, with one extension order for import and require. Bun has an order for each import kind, with .mjs and .cjs in it. With esbuild's order of steps, a key for x.mjs applies to require("pkg/x"), which loads x.js (packagejson/BrowserMapKeyNamesTheFileThatLoads). #47034 fixes the same class of defect for main by a new extension_order parameter. This PR does not touch the main and index lookups, so the two do not depend on each other. Both edit check_browser_map, so the second one to land needs a rebase.

The lookup after resolution. After a bare specifier resolves, main looks the resolved file up in the map once more (check_package_path), when the result has a package_json. That is how a file that exports or imports resolves to gets remapped, and a file below a nested package.json. The result has a package_json only for a file below node_modules/<name>/, so a project file or a workspace package that tsconfig paths resolves does not get this lookup. esbuild has no such lookup. typeorm, yaml@1 and @huggingface/hub depend on it. This PR keeps it, with its package_json condition, and adds one condition: a result that a candidate lookup already handled (MatchResult.browser_remapped) is not looked up again. load_as_file_or_directory takes package_json from the path segment after the last node_modules/, which is node_modules/@scope for a scoped package. So a scoped package never got main's lookup. The candidate lookup covers it now.

#46985 adds the same browser_remapped field for another purpose (a chain {"./main.js": "./a.js", "./a.js": "./b.js"} applies twice on main). This PR does not contain that fix or its 11 tests. The two PRs conflict in 3 files (resolver.rs, result.rs, packagejson.test.ts), so the second one to land needs a rebase.

The empty module.

• An import, require() or import() from a JavaScript file gets it, for a disabled path in the file namespace. Everything else keeps the external record that main has: Bun's stubs for Node.js builtins (node namespace), a stylesheet's @import and url(), an HTML tag, require.resolve, and all of the DevServer.
• Its key is the mark Path.is_disabled_module plus the path text. PathMap (was PathToSourceIndexMap) keeps marked paths in a second table. The module graph and the queue of each importer both use it, so one input gives one bundle (browser/BrowserFieldDisabledAndLiveFileIn*, four orders).
• Its name is the path as the import spells it, without a trailing separator, as in esbuild. ./dir and ./dir/ are one module. pkg/x and pkg/x.js are two, as in esbuild.
• It is an empty source that goes through Bun's existing path for an empty file. No plugin loads it. bun build --check checks the file as written.
• A named import of it is undefined, with esbuild's warning (a debug message when the importer is inside node_modules). esbuild has that as a general rule for a file with no exports. Bun's copy of the rule (LinkerContext.rs, "Is this a named import of a file without any exports?") needs a lazy-export file that does not use module, and every lazy-export file uses module. So it never fires, and the linker asks for the mark here.
• A live main stands in for a module entry that the map disables. The resolver decides that where it builds the pair, before it picks the module type.
• A name that reaches it through export * is undefined too, with a warning. esbuild fails the build there, and main builds it.

Differences from esbuild that stay:

• The lookup after resolution (above).
• A key with an implicit extension applies only in steps 2 and 4 (above). esbuild also takes the key ./dir.js for pkg/dir when dir/ is a directory. Main and this branch load the directory.
• The empty module is CommonJS for every file name. esbuild takes ESM from a .mjs or .mts name: a default import of a disabled node.mjs fails its build (No matching export in "(disabled):node.mjs" for import "default"), and other forms give other values.
• A package whose module entry is disabled and whose main is not: import gets main. esbuild gives {}. Main fails with File not found "".
• export * from a disabled file, then an import of a name that no other export gives: undefined and a warning. esbuild: No matching export.
• With --splitting, import() of a disabled module resolves to { default: {} }, as without splitting. esbuild resolves to {}.
• An entry point that the map disables is a build error, as on main (bundler: fail the build when every entry point is dropped instead of linking zero entry points #39799). esbuild writes an empty file.
• No onLoad or onBeforeParse plugin runs for the empty module. esbuild calls the plugin.
• A stylesheet or an HTML file that names a disabled path keeps the reference as written, and no file is emitted for it. Main does the same where its lookup applies. For a package subpath this is a change: main inlined @import "@s/p/x.css" and failed on @import "u/x.css" (File not found ""). esbuild drops a disabled @import. In a sample of 1,490 packages, 83 have a browser map, and none of them has a stylesheet, HTML or asset key.
• A NODE_PATH package with exports gets the plain file lookup, as on main. Bun has no exports step for NODE_PATH (resolver: apply package.json exports to packages found via NODE_PATH #36566).
• An empty string as a target ("./x.js": "") is stored like false, as main's parser does. esbuild ignores such an entry. No package in the sample has one.
• A package.json with "browser": {} is not a scope of its own, as on main: the map of the project still applies inside that package. In esbuild it does not.
• A map with two keys for one path ("./x" and "x.js", or "./x.js" and "./x/index") can give another answer than esbuild, because of the order above. The table has one key in each layout.
• A relative import (./x) keeps main's single lookup before the file. A package subpath (pkg/x) has the four steps. With sibling files x.js and x.mjs and one key, the two can differ. Main differs more often.

Behavior changes relative to main:

• typeof require(disabled) is "object". import def from disabled is {} and not undefined. import() of a disabled module resolves to its namespace.
• A call of a disabled module throws TypeError (main's stub is callable). iconv-lite 0.4.24 has "./lib/streams": false and runs require("./streams")(iconv) when process.versions.node is set. A default bun build of it prints nothing on both builds. Main's bundle runs under Bun and Node.js, and this branch's bundle throws at load: TypeError: require_streams() is not a function. Packages that load iconv-lite 0.4 (raw-body@2, body-parser@1) inherit that. In a browser the call does not run. esbuild 0.21.5 does not build iconv-lite for the browser without polyfills (Could not resolve "string_decoder"), so there is no esbuild output to compare.
• With --splitting, the empty module is a file of the graph. An import() of a disabled path becomes a chunk of its own, as in esbuild. Two disabled bare names with one display name ((disabled):ws from two places) fail such a build with Multiple files share the same output path.
• A project file that tsconfig baseUrl or an identity paths entry resolves no longer gets the project's map by name (the next item).
• The Browserify bare-name rule is esbuild's: "./x" matches require("x") only inside the package that owns the map, relative to the importer's directory, with no implicit extension. On main "./x.js" matched require("x") by name, also from a dependency. Keys are stored as written (main normalized them, with the native separator on Windows, which resolver: compare browser map keys with forward slashes on every platform #41183 reports).
• A subpath import is checked against the package's map for unscoped and scoped packages and for NODE_PATH.
• require("./node/") matches the key "./node".
• An extensionless target inside node_modules takes .js before .ts (packagejson/BrowserMapTargetExtensionOrderInNodeModules).
• The metafile lists the empty module as an input, (disabled):<path>, and the import of it is not external.

The table test. test/bundler/browser-field-matrix.ts generates 564 layouts: the package location (unscoped, scoped, with exports, the project itself), ten spellings of the key, a file or false as the value, and the import (from outside by six spellings, from inside by relative and bare specifiers, from the package root and from a subdirectory). One build per group gives the files that each import reaches, from the metafile. bundler_browser.test.ts pins the table (6 cases, 5 fail on main). bun scripts/browser-field-matrix.ts builds the layouts with esbuild too: 560 of 564 are equal to esbuild 0.21.5, names of the empty modules included. The 4 others are the exports layouts. Main is equal in 463.

Real packages, built for the browser with main and with this branch: typeorm@0.3.20 (import and require), yaml@1.10.2 (root, yaml/parse-cst, yaml/types, yaml/util), @huggingface/hub@2.17.9 (import and require), @analytics/core@0.13.2, react-select@3.2.0 (root, /async, dist/react-select.esm), axios@1.7.7, nanoid@3.3.7, uuid@9.0.1 and @aws-sdk/token-providers@3.664.0 give byte-identical bundles. Two change as intended: simple-get@4.0.1 tests typeof decompressResponse === 'function' and simple-websocket@9.1.0 tests typeof ws !== 'function' on a module their map disables. On main the websocket fails with function is not a constructor. iconv-lite changes as described above.

Measurements. Release builds of main (9b8347a) and of this branch on that commit, linux-x64.

Bundle bytes, --target=browser, not minified / --minify. The project's package.json has "./node.js": false:

entry	main	this branch	esbuild 0.21.5
const x = require("./node.js")	610 / 386	351 / 165	355 / 104
import x from "./node.js"	64 / 42	87 / 25	1502 / 574
import * as x from "./node.js"	49 / 33	544 / 229	1471 / 566
import { y } from "./node.js"	52 / 36	36 / 21	33 / 21
import "./node.js"	27 / 16	27 / 16	27 / 16
import("./node.js")	63 / 47	1457 / 704	1508 / 597

Binary (size): text 108,863,485 to 108,873,469 (+9,984), data unchanged, bss +72, file +8,192 bytes.

Instructions in user mode, all threads, median of 7 builds (perf_event_open, the outputs are byte-identical):

build	main	this branch	spread of main's 7 runs
20,001 modules, relative imports, browser	1,563.6 M	1,567.0 M (+0.22%)	2.1%
400 packages with a browser map, 800 imports, browser	80.05 M	80.72 M (+0.84%)	2.9%
the same, --target=bun	71.07 M	71.29 M (+0.31%)	1.3%
import { DataSource } from "typeorm", browser	260.00 M	259.79 M (-0.08%)	2.6%

Per call, read from the diff. Targets other than browser: one care_about_browser_field branch per package candidate, one is_disabled check per resolved path in finalize_result, one is_disabled branch per import record. For the browser target, each package candidate also costs one copy of its path into a pooled buffer and one or two check_browser_map calls. A package with no browser map in scope leaves each call at its first check.

Tests. New or enabled cases, and how many fail on a build of main: packagejson.test.ts 60 new (32) and 6 enabled todo cases (6), bundler_browser.test.ts 29 (19), metafile.test.ts 2 (1), bake/dev/bundle.test.ts 1 (1), resolve.test.ts 6 (0). The cases that pass both ways are guards: esbuild's end-to-end cases that main already gets right, the remap cycles, the runtime and the node and bun targets (which ignore the map), the specifier length limits, and the references that stay as written. TestPackageJsonDisabledTypeModuleIssue3367 and TestPackageJsonBrowserMatchingTrailingSlashIssue4187 are ported from esbuild's Go tests. Cases that pin an answer esbuild does not give have skipOnEsbuild with the reason. The two cases of #41183 are here too: they fail on main on Windows only. esbuild's end-to-end cases are in packagejson.test.ts and not in extra.test.ts: the AOT suite draws the inputs of extra.test.ts and builds them for another target, where a browser map is not read.

Self-review. Three review passes ran on three revisions. The last one raised 24 concerns. Fixed here: the main and index lookups use disabled_path, the live main is chosen in the resolver, dead branches and abs_alloc are gone, one helper holds the node_modules warning rule, the table helper no longer blames the wrong layout, the comments name the differences from esbuild, and four test gaps are closed (import() without --splitting, default plus star under --minify-whitespace, the main with a trailing separator, the builtin stub case no longer pins function). The rest are decisions or work outside this diff, and they are in the lists above and below. The review also proposed to land this as two PRs, cut at select_import_module (the resolver port, and the empty module). That is a question for the maintainer, in a comment on this PR.

Not in this PR.

• The DevServer keeps the external record for a disabled path. bun build and the DevServer now give different values for require() of a disabled path ({} and a function). Main's two pipelines agree. An ESM import of a disabled path has no binding in the DevServer on main, and this PR disables more paths (a scoped subpath, a key with no extension). dev server: declare the namespace of an import that the browser target disables #46579 declares that binding and adds a parity test. It should land first, or the second of the two to land updates the expectation.
• Bun's own stubs for Node.js builtins (require("fs") in a browser build) stay a function. A package whose map has "fs": false gets {}, so one bundle can hold both values. bundler: emit an empty object for modules disabled via the browser field #33059 changed the stubs and was closed in favor of this PR. Revision 1 carried that change, and a review took it out as a rider. It needs an owner.
• Main's printer for the stub has defects that the empty module does not have: import d, * as ns under --minify prints no semicolon, and import() of a disabled path is printed as written. They remain for the builtin stubs.
• A browser key spelled node:x is never read (the lookup strips the prefix first). esbuild reads it.
• The page of a server build (--target=bun with an HTML import) uses a resolver that was set up for another target, and gets none of this (bundler: derive the options of a server build's page from the browser target #46548, resolver: keep every main field of a package.json in the shared cache, select per resolver #46652).
• The chain fix of resolver: do not look up a "browser" map target again after resolution #46985, the main extension order of resolver: look up "main" in the "browser" map with the extension order of its load, for script imports #47034, the exports step for NODE_PATH of resolver: apply package.json exports to packages found via NODE_PATH #36566, the --external order of resolver: check user externals before browser node builtin polyfills #35212.
• The string form of browser and which resolver reads it first (resolver: keep every main field of a package.json in the shared cache, select per resolver #46652, Bun can't bundle isomorphic-ws #22887).
• The general linker rule for a file with no exports. It changes every target.

Suites run with the debug build: test/bundler/esbuild/{packagejson,extra,default,importstar,splitting,loader,dce,css,tsconfig}.test.ts, test/bundler/{bundler_browser,bundler_cjs,bundler_edgecase,bundler_npm,bundler_regressions,bundler_splitting,bundler_plugin,bundler_html,bundler_barrel,metafile,bun-build-api}.test.ts, test/js/bun/resolve/{resolve,resolve-error}.test.ts, test/bake/dev/bundle.test.ts, test/bake/dev-and-prod.test.ts. The cases that fail there fail by a time limit of the debug build, in tests this PR does not touch: default/TopLevelAwaitForbiddenRequireSourceMap{API,CLI}, edgecase/DeepImportDiamondDAG, two splitting/FoldingNeverMakesABundleWorse graphs, and three cases of bun-build-api.test.ts (two bytecode: cases and "thousands of times").

---

no test proof · iteration 11 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/resolve/resolve.test.ts, test/bundler/metafile.test.ts

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: 76bd6d9
• Latest commit, not reviewed: 32804a4

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: oven-sh/bun
• Branch: farm/e1cd2044/browser-field-resolution
• File: .coderabbit.yaml
• Commit: 76bd6d9
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 19c62ca6-25d6-407b-9b33-48c2002533b5

Commits

Reviewing files that changed from the base of the PR and between 2a4595b and 76bd6d9.

Files selected for processing: 1

• test/bundler/bundler_browser.test.ts

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

Walkthrough

Browser-field resolution now handles package-subpath remaps, disabled modules, and fallback resolution. Disabled modules use empty JavaScript content and object output. Tests cover resolver behavior, bundler output, and Dev Server resolution.

Changes

Browser mapping and disabled modules

Layer / File(s)	Summary
Browser-map resolution and resolver state src/resolver/...	Browser-map lookup now supports disabled results, package-subpath remaps, extension and index probes, fallback resolution, and bounded remap resolution. The resolver also changes external-package matching order and preserves browser-map key spelling.
Disabled-module bundling and output src/bundler/..., src/js_printer/lib.rs	Disabled paths use distinct source keys and pretty paths. Parsing uses empty JavaScript content. Linking applies CommonJS handling, and printing emits ({}).
Browser-map regression coverage test/bundler/..., test/js/bun/resolve/resolve.test.ts, test/bake/dev/bundle.test.ts	Tests cover browser-map path forms, package boundaries, disabled modules, external imports, plugin behavior, long specifiers, and updated object-stub output.

Suggested reviewers: jarred-sumner, alii

Priority: Normal

Merge Risk: Moderate · up to 76bd6

Code-split dynamic imports of disabled modules can fail at runtime because their namespace lacks the expected default object; this should be fixed before merging.

Pre-merge checks: 4

Check name	Status	Explanation
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.
Title check		The title clearly summarizes the main changes: porting esbuild browser-map lookup and bundling disabled paths as empty modules.
Description check		The description follows the template with both required sections, 'Problem/Fix/Background' content describing what the PR does, and a 'Verified' and test section covering how the change was verified. …

Full details: Description check

Explanation

The description follows the template with both required sections, 'Problem/Fix/Background' content describing what the PR does, and a 'Verified' and test section covering how the change was verified. It is detailed and specific, covering the affected files, behavior changes, measured performance impact, and known downsides. The 'How did you verify your code works?' content is present: it names the test files run, failing-before/passing-after counts, real-package builds, and measurements. Minor gaps: the 'What does this PR do?' section is not labeled with that exact heading (it uses 'Problem', 'Fix', 'Background'), and the verification is partly deferred to CI for two platform-specific test files, which the robobun evidence block flags. These are non-critical, so the description is mostly complete.

Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

• 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[robobun]

Status: ready for review at 32804a4, merged with main at 11c3671. The open decisions for a maintainer are in #40832 (comment).

Reproduced on main with the cases in the PR body: require() of a path that a browser map disables returns a function, require("pkg/foo/bar") ignores the browser map, require("sub") from pkg/sub/foo.js resolves to itself, and a subpath import of a scoped package (@s/p/x.js) is never remapped. All four give esbuild's answer with this branch.

This revision keeps main's browser map lookup after resolution. require("typeorm"), yaml@1.10.2 and @huggingface/hub build to the same bytes as on main.

A table of 564 browser-field layouts is now a test (test/bundler/browser-field-matrix.ts, pinned in bundler_browser.test.ts). bun scripts/browser-field-matrix.ts compares it with esbuild 0.21.5: 560 layouts are equal with this branch and 463 with main. The 4 others are exports targets.

59 of the 104 new or enabled test cases fail without the src/ change. All pass with the debug build.

CI on 32804a4 (build 134535): 184 jobs passed when this was written, and 4 were still queued. The 7 failed jobs are all test-aot-drawn lanes, which are soft failures. Their failing tests are under test/aot/. This PR does not touch them, and the same tests fail on unrelated PRs (builds 134334, 134328, 134327, 134324, 134323, 134322).

[claude]

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

• 🟡 src/bundler/bundle_v2.rs — run_resolver (the plugin-onResolve fallback path) still marks a browser-disabled import as an external is_disabled record instead of materializing the empty CJS module the sibling resolve_import_records path now creates

  Extended reasoning...

  resolve_import_records (line 6658-6864) was updated so a resolve_result.path() == None browser-disabled import becomes an empty CommonJS module keyed by (disabled):<path>, giving import def from x a {} default via __toESM and every require(x) the same shared object. The structurally identical fallback in run_resolver at 2575-2587 — reached whenever an onResolve plugin's filter matched the specifier and the plugin returned undefined (ResolveValue::NoMatch → on_resolve line 4835) — still just sets record.path.is_disabled = true and returns, so the record has source_index = INVALID and no (disabled): pretty. The printer then emits the inline ({}) from print_disabled_import: import def from './node.js' prints var {default: def} = ({}) (def is undefined), import * as ns prints var ns = ({}) (no __toESM, no default), and each require() call site gets a fresh {}. So a browser build with any plugin whose onResolve filter matches the disabled path (e.g. filter: /.*/) yields different semantics for the same import than a build without a…

  Verification: nit — the sibling None arm in run_resolver was not updated to match the new empty-CJS-module handling. At src/bundler/bundle_v2.rs:2575-2587: rust let mut path: Fs::Path<'static> = match resolve_result.path() { Some(p) => *p, None => { let record: &mut ImportRecord = ...; record.path.is_disabled = true; return; } }; This is reached from… | normal —…

• 🟡 src/resolver/resolver.rs — Browser-map subpath check added to only one of two parallel load_node_modules blocks; the auto-install/global-cache path is missing it

  Extended reasoning...

  The new load_package_subpath_from_browser_map call was inserted at line ~2734 in the regular node_modules-walk block, but the structurally identical auto-install/global-cache block (dir_info_for_resolution(.., resolved_package_id) at ~3097, which reaches load_as_file_or_directory(abs_path, ..) at ~3235) was left unchanged. When bun build --target=browser resolves a package through the global cache / auto-install path, require("pkg/foo/bar") with "browser": {"./foo/bar": "./file"} still falls through to the file lookup and either bundles the Node file or fails with Could not resolve — the exact behavior this PR fixes for the normal path but not for its sibling. REVIEW.md requires fixing the whole class (parallel copy-pasted blocks) in the same PR; the fix should call load_package_subpath_from_browser_map(pkg_dir_info, abs_path, ..) before load_as_file_or_directory in both blocks.

  Verification: nit — The structural asymmetry is real. The regular node_modules-walk block gained the new pre-lookup browser-map check at src/resolver/resolver.rs:2733-2750 (if self.care_about_browser_field && self.load_package_subpath_from_browser_map(pkg_dir_info, abs_path, ...).is_success()) immediately before load_as_file_or_directory(abs_path, ..) at :2755. The parallel auto-install/global-cache block…

[robobun]

^{Updated 2:47 PM PT - Oct 28th, 2026}

❌ @robobun, your commit 32804a4 has 10 failures in Build #134535 (All Failures):

• test/aot/perf/generated-code-of-idioms-10.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/joined-programs-limits-lowered-63.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/joined-programs-limits-lowered-18.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-13.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/minifying-first-saves-no-code-9.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-16.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/minifying-first-saves-no-code-10.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-15.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/generated-built-ins-called-by-native-code-with-operands-they-reject.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/programs-that-ask-the-shell-3.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn

---

🧪   To try this PR locally:

bunx bun-pr 40832

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

bun-40832 --bun

[claude]

I reviewed the merged version (d587c94) and did not find any bugs, but with the design change in this push a human look is still worthwhile.

What was reviewed:

• The re-added post-resolution browser-map pass in check_package_path (src/resolver/resolver.rs) with the new browser_remapped gate and relative_remap_scope exclusion; traced browser_remapped through load_package_file_with_browser_map, load_from_main_field, load_as_index and the auto-main path, and checked the removed is_disabled && secondary.is_none() early return is covered by the new !primary.is_disabled guard.
• Scratch-buffer lifetime across the re-entrant remap lookup: abs_path is copied into a pooled buffer before resolve_without_remapping, the 512-byte esm_subpath scratch is saved/restored, and neither load_node_modules call site reads abs_path after the call.
• The bundler side (select_import_module, PathToSourceIndexMap disabled map, ParseTask::init fd/contents selection, scanImportsAndExports Cjs wrap for disabled targets).
• One mismatch between description and code: this push drops the previous tip's "externals before polyfills" check (user_external) and its browser/ExternalWinsOverBuiltinPolyfillAndStub test, so --external=fs with --target=browser is back to the pre-PR stub behavior while the description still says it stays external. Not a regression against main, but the description and the deleted test deserve a stated reason.

Extended reasoning...

The change touches the resolver's "browser" field handling (src/resolver/resolver.rs, ~575 lines), the bundler's module keying and parse-task setup for disabled modules (bundle_v2.rs, ParseTask.rs, PathToSourceIndexMap.rs, LinkerContext.rs, scanImportsAndExports.rs), plus ~1700 lines of tests; it touches no auth, crypto, or injection surface. The latest commit is a merge of main that also reverses an earlier design decision (keeps the post-resolution browser-map pass, gated by a new browser_remapped flag) and silently drops the --external reorder and its test, so the PR description is now stale on that point. The size, the re-entrant threadlocal-scratch resolver paths, and the twelfth round of changes make this a change a human maintainer should still read rather than one safe to auto-approve.

[robobun]

@dylan-conway this revision needs one decision from you.

Your brief asked for esbuild's behaviour. esbuild does not apply the browser map to a file that exports resolves to. Main does. With esbuild's rule, require("typeorm") fails to build for the browser (its exports gives ./index.js, and only the map sends that to ./browser/index.js), yaml@1 takes its Node build, and @huggingface/hub fails to build.

So this revision ports esbuild's lookup and empty module, and keeps main's lookup after resolution for a file that no map lookup picked. packagejson/BrowserMapAppliesToExportsTarget pins that with the shapes of those three packages.

If you want esbuild's rule there, say so and I will remove that lookup and list the packages under Downsides.

[claude]

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

[robobun]

@dylan-conway the head is now e5a4d14. It replaces the earlier revision, and the description is rewritten for it. Five decisions need a maintainer. Each item gives the current choice and the alternative.

1. One PR or two. The change has a clean cut at select_import_module (src/bundler/bundle_v2.rs): the resolver port (which file an import maps to, about +410 lines of src) and the empty module (what a disabled import evaluates to, about +320). Current: one PR, because the request had both. The port alone also sends more paths through main's stub printer, which has defects (no ; after the stub under --minify, import() printed as written). I can split it: the empty module first, then the port.
2. {} in place of the callable stub is a breaking change for a browser bundle that runs under Node.js or Bun. iconv-lite 0.4.24 calls a module that its map disables (require("./streams")(iconv) when process.versions.node is set). Main's bundle runs there. The bundle of this branch throws TypeError: require_streams() is not a function. In a browser the call does not run. Current: {}, as in esbuild. Please confirm, or say if it needs a flag or a release note.
3. Main's lookup after resolution stays. esbuild has none. Without it, require("typeorm") (0.3.20) does not build for the browser and yaml@1.10.2 takes its Node build. For that reason 4 of the 564 layouts in the new table differ from esbuild (a key that names the target of exports).
4. DevServer. It keeps the stub, so bun build gives {} and the DevServer gives a function. An ESM import of a path that only this PR disables has no binding there, which is main's defect for every disabled import. dev server: declare the namespace of an import that the browser target disables #46579 repairs that and should land first. Alternative: host the empty module in the DevServer in a follow-up.
5. Bun's own stubs for Node.js builtins (require("fs") in a browser build) stay a function, and a package with "fs": false in its map gets {}. One bundle can hold both values. bundler: emit an empty object for modules disabled via the browser field #33059 changed the stubs and was closed in favor of this PR. An earlier revision carried that change, and a review removed it as outside the request. Say if you want it here or in its own PR.

Two notes on other open PRs. #46985 and #47034 edit the same resolver lines, so the second one to land needs a rebase. The typed mark on Path is my proposal for the key of the empty module in the bundler's module map. #36549, #41834 and #45248 also change that key, each in another way.

[claude]

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 new thread_local! DISABLED_PATHS in src/resolver/resolver.rs (around line 7307) against the no-thread-local rule: it is a dedup-only intern cache whose keys point into the process-global DirnameStore, so a miss on another thread only appends a second copy of the same path, never a wrong or stale Path.

Extended reasoning...

The re-cut touches the resolver's browser-map lookup, the bundler's disabled-module keying, and ~2,500 lines of tests; it touches no auth, crypto or injection surface. One pre-existing finding is posted inline; this note records the thread-local intern map examined and ruled out this run.

[claude]

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

[claude]

🔴 pre-existing, widened: DevServer users whose app imports a package subpath that the package's "browser" map disables now get a runtime ReferenceError where the base served the file. select_import_module at src/bundler/bundle_v2.rs:6481 returns None for every dev_server build, so the record becomes an is_disabled external and convertStmtsForChunkForDevServer.rs:81 drops the whole import statement without declaring its bindings. Fix: in the dev server give a disabled JavaScript import the same empty module (or declare its namespace as {} like the IS_UNUSED branch does) so named and default bindings exist, rather than dropping the statement. The PR notes this and points to #46579, but nothing in this branch prevents it.

Why this was flagged

Trigger: a Bun DevServer app has import { x } from "@ s/pkg/node" and node_modules/@ s/pkg/package.json contains "browser": {"./node.js": false}. On the base branch the scoped package never received the post-resolution browser lookup, so node.js was bundled and x was defined. With this branch load_package_file_with_browser_map (src/resolver/resolver.rs:5440-5453) remaps the loaded file through remap_loaded_file and returns a disabled primary. In resolve_import_records, select_import_module at src/bundler/bundle_v2.rs:6481 returns None because self.dev_server.is_some(), so src/bundler/bundle_v2.rs:7194-7197 sets import_record.path.is_disabled = true. convert_stmts_for_chunk_for_dev_server then continues on the disabled record at src/bundler/linker_context/convertStmtsForChunkForDevServer.rs:81-83, so no namespace var is declared for the import and the module body throws ReferenceError when it reads x. The same drop already hit paths the base disabled, so the mechanism is pre-existing; this diff widens the population to scoped packages, ./foo/bar keys without extension, and NODE_PATH packages.

Verification: select_import_module at src/bundler/bundle_v2.rs:6481 if self.dev_server.is_some() || !is_javascript_import || !path.is_file() { return None; }, and the caller at 7194-7198 sets import_record.path.is_disabled = true. src/bundler/linker_context/convertStmtsForChunkForDevServer.rs:81-83 if record.path.is_disabled { continue; } skips the SImport entirely. The non-dev bun build path correctly gets the empty module (6484-6487).

[robobun]

Correct, and the description lists it under Downsides. On main the DevServer drops the binding of every disabled import. This PR disables more paths (a scoped subpath, a key without an extension, a NODE_PATH package), so more imports reach that defect.

The repair is #46579. It declares the binding in convertStmtsForChunkForDevServer.rs and keeps stylesheets apart with a record flag, because the DevServer also sets path.is_disabled for a stylesheet. I did not copy those hunks here, so that the two PRs do not overlap.

The order of the two is decision 4 in #40832 (comment): #46579 lands first, or I carry its hunks and tests in this PR. This thread stays open until a maintainer answers.

[claude]

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

Still open from earlier reviews (1):

• 🔴 src/bundler/bundle_v2.rs:6481 — pre-existing, widened: DevServer users whose app imports a package subpath that the package's "browser" map disables no…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• 🔴 src/bundler/bundle_v2.rs:6481 — pre-existing, widened: DevServer users whose app imports a package subpath that the package's "browser" map disables no…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[robobun]

A note on stylesheets for the decision list above. It adds a sixth item.

6. A stylesheet @import of a disabled path. Main prints it as written (@import "pkg";, exit 0), and this PR keeps that ("Differences from esbuild that stay"). esbuild 0.21.5 and 0.25.1 print no rule. The choices are: print as written, print nothing, or fail the build.

The cause is one guard in the CSS import order pass (src/bundler/linker_context/findImportedFilesInCSSOrder.rs:240). It tests ImportRecordFlags::IS_INTERNAL, which only the JavaScript parser sets. esbuild tests WasLoadedWithEmptyLoader there (internal/linker/linker.go:3458 in v0.25.1).

I built the change of that guard to !record.path.is_disabled with 12 tests. The branch is robobun/82b7b627/css-drop-disabled-import (df53fcc). I did not open a PR for it:

• No input is known. None of the 1,370 packages in node_modules and test/node_modules of this repository has a stylesheet key in a browser map. I found no issue that reports it. css: keep a url() in an at-rule prelude as written, and never print the resolved path of a url() #45604 and css: keep the earlier copy of a file with a conditional layer in every import order #46934 were not taken for that reason.
• path.is_disabled has more producers than the map. With the guard, @import "fs" and @import "node:fs" (Bun's stubs for Node.js builtins) also print no rule. With --external fs, main and esbuild keep the rule and the guard drops it (the defect of Flagging Node Dependencies with --external does not work #2701, see resolver: match exact external names before any lookup #47455).
• @import "./node.css" layer(foo); then leaves no @layer foo;, as in esbuild. A real empty file leaves one, so the layer order of the bundle changes.
• Three other readers of the same record stay as on main. url() prints the specifier (esbuild prints url()). An HTML tag stays as written. The metafile lists the import as external: true.

If the answer is esbuild's output, I suggest a mark of its own on the record. This PR can set it where it selects the empty module, and all four readers can test it. The raw is_disabled bit is too wide for that.