resolver: consult browser map for node_modules subpath imports before extension resolution by robobun · Pull Request #36642 · oven-sh/bun

robobun · 2026-08-01T05:33:09Z

What

import 'demo-pkg/no-ext' where demo-pkg/package.json has "browser": {"./no-ext": "./no-ext-browser.js"} was resolving to no-ext.js (the Node file) instead of no-ext-browser.js under --target=browser.

mkdir -p node_modules/demo-pkg
cat > node_modules/demo-pkg/package.json <<'JSON'
{"browser": {"./no-ext": "./no-ext-browser.js"}}
JSON
echo 'export let value = "node"'    > node_modules/demo-pkg/no-ext.js
echo 'export let value = "browser"' > node_modules/demo-pkg/no-ext-browser.js
echo 'import {value} from "demo-pkg/no-ext"; console.log(value)' > entry.js
bun build entry.js --target=browser   # bundles "node", should bundle "browser"

Why

load_node_modules went straight from the exports-map check to load_as_file_or_directory(abs_path), so the browser map was only consulted by the post-resolution check in check_package_path, at which point the path already has .js appended and a "./no-ext" key can no longer match.

esbuild's tryToResolvePackage runs checkBrowserMap(pkgDirInfo, absPath, absolutePathKind) between those two steps (resolver.go):

if pkgDirInfo := r.dirInfoCached(absPkgPath); pkgDirInfo != nil {
    // Check the "exports" map
    if packageJSON := pkgDirInfo.packageJSON; packageJSON != nil && packageJSON.exportsMap != nil {
        ...
    }
    // Check the "browser" map
    if remapped, ok := r.checkBrowserMap(pkgDirInfo, absPath, absolutePathKind); ok {
        if remapped == nil {
            return PathPair{Primary: logger.Path{Text: absPath, ..., Flags: logger.PathDisabled}}, ...
        }
        if remappedResult, ok, ... := r.resolveWithoutRemapping(pkgDirInfo.enclosingBrowserScope, *remapped); ok {
            return remappedResult, ...
        }
    }
}
// Try node's old package resolution rules
if absolute, ok, ... := r.loadAsFileOrDirectory(absPath); ok { ... }

This change adds the equivalent check at the same position.

The remap target can itself be a bare package name ("./sub": "other-pkg"), which makes resolve_without_remapping recurse into load_node_modules. That recursion overwrites the thread-local node_modules_check and esm_subpath buffers that abs_path and esm_.subpath borrow from, so both are recomputed from stable inputs before falling through when the remap resolve fails. The recursion is also bounded via a browser_remap_depth counter so a cyclic remap like {"./x": "pkg-a/x"} falls through to the normal resolve error instead of overflowing the stack (esbuild is unbounded here and stack-overflows on the same input; released bun errors cleanly because it never takes this edge).

#36620 fixed the adjacent load_from_main_field / load_as_index_with_browser_remapping cases (wrong argument to an existing check); this is the follow-up it names for bare subpath imports, where the check was missing entirely.

Verification

All outputs match esbuild (except the cycle case, which esbuild crashes on):

import	browser key	esbuild	bun before	bun after
`demo-pkg/no-ext`	`"./no-ext": "./no-ext-browser.js"`	browser	node	browser
`demo-pkg/no-ext.js`	same	node	node	node
`demo-pkg/no-ext` (dir)	`"./no-ext": "./no-ext-browser/index.js"`	browser	node	browser
`demo-pkg/disabled`	`"./disabled": false`	disabled	loaded	disabled
`demo-pkg/to-pkg`	`"./to-pkg": "other-pkg"`	other-pkg	node	other-pkg
`demo-pkg/to-missing`	`"./to-missing": "missing-pkg"`	falls through	falls through	falls through
`pkg-a/x`	`"./x": "pkg-a/x"` (cycle)	stack overflow	resolve error	resolve error
`demo-pkg/sub`	`exports` + `browser` both set	exports wins	exports wins	exports wins

Un-todos packagejson/BrowserNodeModulesNoExt and packagejson/BrowserNodeModulesIndexNoExt, and adds SubpathDisabled, SubpathToPackage, SubpathToPackageCycle, and ExportsShadowsBrowserSubpath. The first four fail on the released build and pass with this change; the last two are regression guards that pass on both. The rest of packagejson.test.ts is unchanged (84 pass, 7 todo).

… extension resolution import 'demo-pkg/no-ext' where demo-pkg/package.json has browser: {"./no-ext": "./no-ext-browser.js"} was resolving to no-ext.js instead of no-ext-browser.js under --target=browser. load_node_modules went straight from the exports-map check to load_as_file_or_directory, so the browser map was only consulted after the extension had been appended. esbuild's tryToResolvePackage runs checkBrowserMap(pkgDirInfo, absPath, absolutePathKind) between those two steps; add the equivalent check. The new check may recurse back into load_node_modules when the remap target is a bare package name, which overwrites the thread-local node_modules_check and esm_subpath buffers. Recompute both from stable inputs before falling through on a failed remap resolve. Un-todos packagejson/BrowserNodeModulesNoExt and packagejson/BrowserNodeModulesIndexNoExt, and adds coverage for the disabled (false) and package-path remap cases.

coderabbitai · 2026-08-01T05:33:14Z

Walkthrough

Changes

Browser-map package resolution now supports bounded recursive remapping, disabled targets, failed-remap retries, and state restoration. Package JSON tests cover extensionless paths, index resolution, cycles, package remaps, disabled subpaths, and exports precedence.

Browser-map resolution

Layer / File(s)	Summary
Resolver state and retry preparation `src/resolver/resolver.rs`	The resolver tracks remap depth. Package specifiers and candidate paths can be rebuilt after recursive resolution.
Recursive browser-map remapping `src/resolver/resolver.rs`	Node-module lookup handles disabled and recursive remaps, limits nesting to 32 levels, restores state, and retries failed package-path remaps.
Browser-map behavior coverage `test/bundler/esbuild/packagejson.test.ts`	Tests enable extensionless and index cases and cover disabled subpaths, package remaps, cycles, and `exports` precedence.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the resolver change for browser maps on node_modules subpath imports before extension resolution.
Description check	✅ Passed	The description explains the problem, rationale, implementation, affected behavior, and detailed verification results.
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.

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

robobun · 2026-08-01T05:33:44Z

^{Updated 3:05 AM PT - Aug 1st, 2026}

❌ @robobun, your commit b2d25a9 has 3 failures in Build #86882 (All Failures):

test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan
test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts - SIGABRT on 🐧 13 x64-asan
test/cli/install/bun-upgrade.test.ts - code 1 on 🪟 11 aarch64

🧪 To try this PR locally:

bunx bun-pr 36642

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

bun-36642 --bun

claude

No bugs found, but this adds ~85 lines of new control flow to load_node_modules with manual bufs! slot recomputation after recursion — resolver-critical enough that a human familiar with the buffer-lifetime conventions here should sign off.

What was reviewed:

Placement mirrors esbuild's tryToResolvePackage (browser-map check between exports and loadAsFileOrDirectory); disabled/remap/fallthrough branches match.
bufs!(node_modules_check) / bufs!(esm_subpath) are only re-entered via the package-path branch of resolve_without_remapping, so gating the recompute on remap_is_package_path covers the clobber; the relative branch uses bufs!(resolve_without_remapping).
Package<'a> is Copy, so switching if let Some(ref esm) → if let Some(esm) doesn't move out of esm_ across loop iterations.

Extended reasoning...

Overview

Adds a "browser" map check inside load_node_modules (src/resolver/resolver.rs) between the exports handling and the fallthrough to load_as_file_or_directory, so a bare subpath import like demo-pkg/no-ext matches a "./no-ext" browser-map key before extension resolution appends .js. Un-todos two ported esbuild tests and adds two new ones (false remap and remap-to-package) in test/bundler/esbuild/packagejson.test.ts.

Security risks

None. This is bundler/resolver path selection driven by a package's own package.json; no new untrusted-input surface, no filesystem writes, no privilege boundaries crossed.

Level of scrutiny

High. Module resolution is on the critical path for both bun build and the runtime — a wrong match here silently loads the wrong file for every user. The change also touches the thread-local bufs! scratch-buffer scheme (slices laundered to &'static), where the recursion into resolve_without_remapping can overwrite the backing storage of abs_path and esm_.subpath. The author handles this by recomputing both from stable inputs (dir_info.abs_path, import_path) when the remap target is itself a package path, and I verified that only the package-path branch of resolve_without_remapping re-enters load_node_modules (the relative branch writes to a distinct bufs!(resolve_without_remapping) slot). check_browser_map returns &'static [u8] so remap itself survives the recursion, and remap_is_package_path is captured before the call.

Other factors

The change is a faithful port of the corresponding esbuild block (cited in the PR body), and the four tests cover the happy path, the disabled path, remap-to-package, and remap-to-missing-package fallthrough. The comment-cop bot flagged verbose comments three times; all were shortened and the threads are resolved. Package<'a> is #[derive(Copy)], so the ref → by-value change in the if let is sound across the outer loop. Given the subtlety of the buffer-clobber handling and that this is core resolver logic, I'm deferring rather than approving.

… tests A browser map like {"./x":"pkg-a/x"} sends resolve_without_remapping back into load_node_modules with no bound. Cap browser_remap_depth at 32 so a cyclic remap falls through to the clean resolve error instead of overflowing the stack. Add a regression test for the cycle and a test that pins exports precedence over the browser map for subpath imports.

claude

No issues found, but this adds ~90 lines of new control flow to load_node_modules — the resolver's hot path — including a recursion-depth guard and recomputation of thread-local scratch buffers that a recursive resolve_without_remapping clobbers. That's subtle enough to warrant a human look.

What was reviewed

Placement matches esbuild's tryToResolvePackage (browser-map check between exports-map and load_as_file_or_directory).
remap is &'static [u8] from check_browser_map, so it survives the recursion; only abs_path/esm_ need recomputing, and only on the package-path branch (relative remaps go through bufs!(resolve_without_remapping), not the two clobbered slots).
Package<'a> is Copy, so the Some(ref esm) → Some(esm) change is sound and required for the later reassignment.
Cycle guard, disabled (false) remap, exports-precedence, and fall-through-on-missing-package are each covered by a dedicated test.

Extended reasoning...

Overview

Adds a browser-map lookup in load_node_modules (src/resolver/resolver.rs:2820-2903) between the exports-map check and the load_as_file_or_directory fallback, mirroring esbuild's tryToResolvePackage. This lets "browser": {"./no-ext": ...} match a bare demo-pkg/no-ext import before extension resolution appends .js. Also adds a browser_remap_depth: u8 field on Resolver to bound recursion when a remap target is itself a package path, and un-todos two esbuild-ported tests plus adds four new ones (disabled subpath, remap-to-package, self-cycle, exports-shadows-browser).

Security risks

None. This is module-resolution ordering; no untrusted input parsing, auth, or filesystem-escape surface is introduced.

Level of scrutiny

High. load_node_modules is the core of both the bundler's and the runtime's module resolution. The change is behaviorally correct against esbuild (per the PR's verification table), but the implementation leans on non-obvious invariants: abs_path and esm_.subpath borrow from thread-local bufs! slots that a recursive load_node_modules call overwrites, so they're recomputed after a failed package-path remap. I verified those two slots are only written inside load_node_modules (grep confirms), so gating the recompute on remap_is_package_path is sufficient — the relative-remap branch of resolve_without_remapping uses a separate resolve_without_remapping slot and load_as_file_or_directory doesn't touch either. Still, this is exactly the class of change where a maintainer familiar with the resolver's buffer discipline should sign off.

Other factors

The comment-cop bot flagged the buffer-recompute comment three times; the author reduced it to a single line, which now reads as load-bearing rather than a workaround justification. Package<'a> is #[derive(Copy)] so moving from Some(ref esm) to Some(esm) is a copy, not a move — needed because esm_ is now reassigned. The 32-deep recursion limit is arbitrary but consistent with similar guards elsewhere; the cycle test exercises it. Test coverage is good and each row of the verification table maps to an assertion.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/resolver/resolver.rs`:
- Around line 2819-2903: Replace the literal browser remap depth bound in the
resolver block guarded by care_about_browser_field with a named constant set to
32. Define the constant near the related resolver constants and add a brief
comment documenting that it prevents cyclic browser-field remaps; use it in the
browser_remap_depth comparison without changing the recursion behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 06926954-ef51-4dff-890d-eeaaf62c6d68

📥 Commits

Reviewing files that changed from the base of the PR and between 5f7e62d and b2d25a9.

📒 Files selected for processing (2)

src/resolver/resolver.rs
test/bundler/esbuild/packagejson.test.ts

claude

No bugs found, but this adds ~90 lines of new control flow (including bounded recursion and manual thread-local buffer restoration) inside load_node_modules, which is a hot and subtle resolver path — worth a human pass on the buffer-reuse invariants.

What was reviewed:

Placement matches esbuild's tryToResolvePackage: browser-map check sits between the exports-map early return and load_as_file_or_directory, so exports still wins (covered by ExportsShadowsBrowserSubpath).
Buffer recompute is correctly gated on remap_is_package_path: the relative-remap branch of resolve_without_remapping writes to bufs!(resolve_without_remapping), not node_modules_check/esm_subpath, so abs_path stays valid for the fall-through when the remap is relative.
Recursion depth counter (browser_remap_depth < 32) prevents the self-cycle stack overflow; test SubpathToPackageCycle pins the error path.
All new early returns restore extension_order and decrement the debug indent, matching sibling exit sites.

Extended reasoning...

Overview

The PR inserts a check_browser_map call into load_node_modules between the package exports handling and the load_as_file_or_directory fallback, mirroring esbuild's tryToResolvePackage. It handles three remap outcomes: false (disabled → return a is_disabled path), a resolvable target (return via resolve_without_remapping), and an unresolvable target (fall through to normal resolution). Because a bare-package remap target re-enters load_node_modules and overwrites the node_modules_check / esm_subpath thread-local buffers, the fall-through path recomputes abs_path and esm_ from stable inputs. A new browser_remap_depth: u8 field bounds the recursion at 32. Two .todo esbuild-ported tests are un-skipped and four new tests are added covering the disabled, remap-to-package, cycle, and exports-precedence cases.

Security risks

None identified. This is bundler-time module resolution; the browser map is package-author-controlled metadata that already flows through check_browser_map at other call sites. The new recursion is bounded so a malicious cyclic remap cannot overflow the stack (the released bun already errored here; esbuild is the one that crashes).

Level of scrutiny

High. load_node_modules is on the critical path for every bare import in every bundle, and the function relies on a set of shared thread-local scratch buffers (bufs!(...)) with implicit non-overlap invariants. The change deliberately introduces a recursive edge that violates one of those invariants and then compensates by recomputing the clobbered slots. I traced the buffer usage: the recompute is correctly gated on remap_is_package_path because only the package-path branch of resolve_without_remapping re-enters load_node_modules; the relative branch writes to a distinct bufs!(resolve_without_remapping) slot. The is_self_reference case correctly skips the abs_path recompute since it points at arena-stable dir_info.abs_path rather than a buffer. Still, this is exactly the kind of invariant a maintainer who owns the resolver should sign off on — the "clobbered buffers" pattern is fragile enough that a future refactor of resolve_without_remapping could silently break it.

Other factors

Test coverage is good: the un-todo'd tests are direct esbuild ports, and the new tests cover each row of the verification table including the negative regression guards. The comment-cop bot's complaints about long comments were addressed in follow-up commits. The .expect("oom") on dirname_store.append_slice matches existing convention in the file for that arena. I'm deferring rather than approving because the buffer-restoration workaround, while it reads correct, is the kind of subtle cross-function invariant that benefits from a second pair of eyes from someone who maintains this file.

robobun · 2026-08-01T08:33:55Z

The resolver change itself is green: test/bundler/esbuild/packagejson.test.ts passes on every lane in both builds 86799 and 86882.

Remaining red lanes are unrelated to this diff (resolver touches nothing in these areas):

bun-upgrade.test.ts on Windows aarch64 (CLI upgrade)
worker-transfer-terminate-stress.test.ts / test-worker-message-port-transfer-terminate.js SIGABRT on debian x64-asan (worker_threads)
s3.test.ts R2 content-type + file-watcher EAGAIN on alpine aarch64 (build 86799)
36577.test.ts frozen-lockfile on Windows, pre-existing on main
assorted tests that passed on retry

Ready for review.

robobun · 2026-08-28T17:21:54Z

#40832 includes this lookup (browser map checked before the file lookup in load_node_modules) together with the rest of the browser-field fixes. If that lands, this one can be closed.

#36642 and #37532 each fix one part of the "browser" field handling that this branch reworks. Their tests are added here so that closing those PRs loses no coverage: - packagejson/BrowserNodeModulesSubpathDisabled, SubpathToPackage, SubpathToPackageCycle and ExportsShadowsBrowserSubpath (#36642): a package subpath import whose map entry is false, a bare package, a cycle, or shadowed by "exports". - "browser map lookup of a bare specifier that does not fit a path buffer" in test/js/bun/resolve/resolve.test.ts (#37532): a specifier longer than a path buffer must fail to resolve, not abort the build. On main, four of the six packagejson cases and four of the five resolve.test.ts cases fail. All pass with this branch.

robobun · 2026-08-29T06:59:18Z

Superseded by #40832, which reworks the browser field to follow esbuild and covers this case. Its load_node_modules checks the package's browser map against the extensionless subpath before the file lookup, the same position as esbuild's checkBrowserMap.

The tests from this PR now live on that branch (bdb312e2b6): packagejson/BrowserNodeModulesNoExt and packagejson/BrowserNodeModulesIndexNoExt are enabled there, and packagejson/BrowserNodeModulesSubpathDisabled, packagejson/BrowserNodeModulesSubpathToPackage, packagejson/BrowserNodeModulesSubpathToPackageCycle and packagejson/ExportsShadowsBrowserSubpath are added. All six pass with that branch's debug build, and four of them fail on main.

Closing in favor of that PR.

github-actions Bot added the claude label Aug 1, 2026

[autofix.ci] apply automated fixes

b49be90

github-actions Bot reviewed Aug 1, 2026

View reviewed changes

Comment thread src/resolver/resolver.rs Outdated

github-actions Bot reviewed Aug 1, 2026

View reviewed changes

Comment thread src/resolver/resolver.rs Outdated

shorten code comments

59918bb

github-actions Bot reviewed Aug 1, 2026

View reviewed changes

Comment thread src/resolver/resolver.rs Outdated

shorten buffer comment to one line

ea4bf94

claude Bot reviewed Aug 1, 2026

View reviewed changes

ci: retrigger

b2d25a9

coderabbitai Bot reviewed Aug 1, 2026

View reviewed changes

Comment thread src/resolver/resolver.rs

claude Bot reviewed Aug 1, 2026

View reviewed changes

robobun mentioned this pull request Aug 28, 2026

bundler: port esbuild's "browser" map lookup and bundle a disabled path as an empty module #40832

Open

robobun closed this Aug 29, 2026

Navigation Menu

Repository navigation

resolver: consult browser map for node_modules subpath imports before extension resolution - #36642

resolver: consult browser map for node_modules subpath imports before extension resolution#36642
robobun wants to merge 6 commits into
mainfrom
farm/fbddaf23/resolver-browser-map-node-modules-subpath

robobun commented Aug 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 1, 2026 •

edited

Loading

Uh oh!

robobun commented Aug 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Aug 1, 2026

Uh oh!

robobun commented Aug 28, 2026

Uh oh!

robobun commented Aug 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

robobun commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Verification

Uh oh!

coderabbitai Bot commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

robobun commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Aug 1, 2026

Uh oh!

robobun commented Aug 28, 2026

Uh oh!

robobun commented Aug 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Aug 1, 2026 •

edited

Loading

coderabbitai Bot commented Aug 1, 2026 •

edited

Loading

robobun commented Aug 1, 2026 •

edited

Loading