Repository navigation
Conversation
…form The browser map keys were normalized with the native separator when the package.json was parsed, so on Windows a key like "pkg/sub" was stored as pkg\sub. The lookup for a package path uses the import specifier as written, with forward slashes, so a key with a slash never matched on Windows. Store the keys with forward slashes and compare a path inside the package in the same form, as esbuild does.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughBrowser-field path handling now uses POSIX normalization for consistent slash matching. Resolver comparisons and index probing use POSIX semantics. Bundler tests cover disabled and redirected package subpath mappings. ChangesBrowser-map path handling
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized resolver change makes browser-map keys match consistently across platforms and is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:34 AM PT - Sep 2nd, 2026
✅ @robobun, your commit f3178ae164b960dd39520c123328494e2ff003ad passed in 🧪 To try this PR locally: bunx bun-pr 41183That installs a local version of the PR into your bun-41183 --bun |
|
Status Reproduced on Windows (x64, bun 1.4.1) with The two new tests fail on Windows with bun 1.4.1 and with a debug build that has CI (build 109366): Self-review: 1 concern on the first version (a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and found no bugs. The resolver is a hot shared path and the observable fix is Windows-only (the new tests pass on Linux with or without the change), so a human confirming the Windows CI lane is worthwhile.
What was reviewed
- Traced
normalize_string_buf::<_, Posix, _>→normalize_string_loose_buf_t, which usesis_sep_any_ton input and writes/— so theAbsolutePathcase on Windows (foo\bar.jsafter theabs_pathstrip) still normalizes tofoo/bar.jsand matches the now-/-keyed map. - POSIX behavior is byte-identical:
platform::AutoaliasesPosixon unix,fs.normalize_bufwas already<false, Auto, false>, andSEP/SEP_STR/Platform::AUTOwere already/there. - The new
input_path = cleanedrebinding forAbsolutePathis a no-op on POSIX (resolved abs paths have no./..after the prefix strip); on Windows it's what makes the\→/conversion reach themap.getprobe. check_path's index-probe inputs are all forward-slash after this change (raw specifier,cleaned, or./+either), so trimming only/and joining withPlatform::Posixis consistent.
Extended reasoning...
Overview
The PR fixes package.json browser field remapping on Windows for keys containing a slash ("pkg/sub", "@ scope/pkg"). Three files change: src/resolver/package_json.rs swaps key normalization from platform::Auto to platform::Posix; src/resolver/resolver.rs swaps the lookup-side normalization to Posix, rebinds input_path to the cleaned form for the AbsolutePath kind, and switches the <path>/index probe from native SEP/SEP_STR/Platform::AUTO to hardcoded / and Platform::Posix; test/bundler/bundler_browser.test.ts gains two itBundled cases covering disable and remap of a slash-containing key.
Security risks
None. This is a build-time module resolution comparison — no untrusted input crosses a trust boundary, no allocation sizing changes, no unsafe blocks are added. The existing unsafe { Interned::assume } sites in check_path are unchanged.
Level of scrutiny
Medium-high. The resolver runs for every import in every bundle, so a mistake here is high blast radius. I read normalize_string_buf_t and confirmed the non-obvious fact this change hinges on: Platform::Posix in that function dispatches to normalize_string_loose_buf_t, which recognizes both separators on input (is_sep_any_t) and emits /. That is what makes the AbsolutePath case still work on Windows after the key side switches to /. I also verified that on POSIX every swapped constant/type resolves to the same value it did before (Auto == Posix, SEP == '/', Platform::AUTO == Posix), so unix behavior is byte-for-byte unchanged. The join_string_buf call goes through the file-local value-dispatch shim (line 59 mod bun_paths), so only the argument value changed, not the call shape.
Other factors
The tests are in the right file, use itBundled, are hermetic, and assert both runtime stdout and bundle contents. Per the PR description they pass on Linux with and without the fix — expected, since the bug is Windows-only — which means the fail-before proof lives only in the Windows CI lane. That, plus the resolver's criticality, is the reason to defer rather than approve outright: someone should eyeball the Windows job for bundler_browser.test.ts and the sibling suites the description lists (esbuild/packagejson, bundler_edgecase, bun/resolve). The comments added are on the informative side but each answers a "why Posix here?" question a future reader would otherwise have to trace through resolve_path.rs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Resolver: - Restore the `package_json` condition of the lookup after resolution. Remove the chain marks and the tests that belong to #46985. - A package candidate takes a key in this order: a key for the path as the import spells it, the key of the file that loads, the lookups of a directory, then a key with an implicit extension for a file that does not exist. A key for `x.mjs` no longer applies to a `require` that loads `x.js`. - One constructor for a disabled path (`disabled_path`): no trailing separator, and one interned copy of each distinct path. The `main` and index lookups use it too. - A live `main` stands in for a disabled `module` entry where the pair is built, so `finalize_result` sees the path that loads. - A remap cycle stops at the entry that is already being resolved. - The `esm_subpath` scratch is on the frame of `load_node_modules`. Bundler: - The empty module of a disabled file has a typed mark, `Path.is_disabled_module`. `PathMap` keeps marked paths in a second table, for the module graph and for the queue of each importer. One input gives one bundle. - `export *` from a disabled file gives no name. An import of such a name is `undefined` with a warning, and not a build error. Tests: - A table of 564 layouts (`test/bundler/browser-field-matrix.ts`) with a snapshot, and `scripts/browser-field-matrix.ts` to compare it with esbuild. - Cases for each decision above, and the two cases of #41183.
Problem
browsermap key with a slash never matches a bare import under--target=browser.{"pkg/sub": false}and{"@scope/pkg": false}bundle the Node file, and{"pkg/sub": "./shim.js"}is ignored. Linux and macOS apply the same entries.parse_package_jsonnormalized each key with the native separator (src/resolver/package_json.rs:617), sopkg/subwas stored aspkg\sub.check_browser_map(src/resolver/resolver.rs:5042) compares a package path as written, with forward slashes.Fix
<path>/indexprobe with forward slashes.AbsolutePathmode) is compared in its normalized form, sofoo\bar.jsmatches./foo/bar.json Windows. A package path is compared as written. esbuild does the same: keys go throughpath.Cleanand the relative path gets forward slashes.test/bundler/bundler_browser.test.ts(2 new cases) on Linux and Windows. Alsoesbuild/packagejson,bundler_edgecaseandbun/resolveon both, plusbundler_npmand the bakebrowser field is usedtest.Background
browserfield maps a specifier, or a file in the package, to another file or tofalse(an empty module). Bundlers apply it for--target=browser.check_browser_maphas two modes.PackagePathtakes the import specifier as written.AbsolutePathtakes a resolved file path and strips the package directory.Notes
Repro on Windows with bun 1.4.1,
bun build --target=browser entry.js:Linux prints
var { default: x} = (() => ({}));. Windows bundlesnode_modules/pkg/sub.js. Every scoped package key (@scope/pkg) has a slash, so those are affected too. A slash-free key such as"pkg": falseworks on both.Why only the
PackagePathmode broke:DirInfo.abs_pathends with a separator, so theAbsolutePathinput is the path below the package directory, with native separators on both sides of the lookup. The bare exact match for a package path was the only probe that compared a raw specifier against a normalized key.The first version of this change added
map.get(cleaned)as a fallback, which is the input cleaning esbuild removed in 0.11.16 (#1209, over-matching). The self-review pointed that out, so the change now mirrors esbuild's current shape instead. A specifier with redundant segments (pkg/./sub) still does not match the keypkg/sub, on any platform, as before.The fail-before can only be observed on Windows. The gate runs the proof on Linux, where the two tests pass with and without the fix. Windows proof: with
src/resolver/checked out fromorigin/mainand the rest of the branch in place,bun bd test test/bundler/bundler_browser.test.ts -t "BrowserField.*PackageSubpath"fails both tests (string stringandnode-only-subinstead ofundefined undefinedandshim). With the branch'ssrc/, both pass.Suites run:
test/bundler/bundler_browser.test.ts(27 tests, Linux and Windows),test/bundler/esbuild/packagejson.test.ts,test/bundler/bundler_edgecase.test.ts,test/js/bun/resolve/resolve.test.ts(Linux and Windows),test/bundler/bundler_npm.test.ts(Linux),test/bake/dev/esm.test.ts -t "browser field is used"(Linux and Windows).[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file