Repository navigation
Conversation
The NODE_PATH search joined each entry with the raw specifier and called load_as_file_or_directory, which only looks at main/index. A package with an "exports" field found via NODE_PATH therefore had exports ignored: exports["."] lost to "main", mapped subpaths did not resolve, and unexported/null-blocked files loaded. Node treats NODE_PATH entries as node_modules-like search roots and applies PACKAGE_EXPORTS_RESOLVE there. Extract the exports-map probe (previously inlined in the node_modules walk) into resolve_from_package_exports and call it for each NODE_PATH entry before the legacy fallback. The node_modules walk now calls the same helper, and packages without an "exports" field fall through unchanged.
WalkthroughChangesPackage Package exports resolution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:15 AM PT - Jul 31st, 2026
✅ @robobun, your commit 51a1626fbb1d7e3aee69329306ae18ad5fc5ecf8 passed in 🧪 To try this PR locally: bunx bun-pr 36566That installs a local version of the PR into your bun-36566 --bun |
Bun consults NODE_PATH for ESM imports too (Node does not), and that channel shares the same load_node_modules path as require. Add an import() probe alongside the require() probe so the test exercises both.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 3542-3550: Add a regression test covering CSS package exports
resolution for ImportKind::At and ImportKind::AtConditional, using a package
whose exports map distinguishes style from import conditions. Assert that a CSS
`@import` resolves through the style condition and selects the style-specific
target.
- Line 3528: In handle_esm_resolution, initialize the local module_type to
options::ModuleType::Unknown and remove both assignments that overwrite
out.module_type with module_type. Preserve the module type resolved from the
package enclosing the final file, including nested packages with a different
type.
- Around line 2691-2695: Update the ESM lookup in the resolver flow around
dir_info_cached to use the checked absolute-path join for the NODE_PATH entry
instead of abs_buf. When the checked operation returns None, skip only the ESM
exports lookup and preserve the later checked fallback path unchanged.
In `@test/js/bun/resolve/resolve.test.ts`:
- Around line 444-468: Update the CommonJS and ESM probe catch blocks to return
the underlying resolver error code or message instead of the generic "ERR", then
assert the concrete expected failure for each negative case, including exports
encapsulation and the "./priv/*": null block. Extend the ESM probe in probe.mjs
with `@scope/pkg` and legacy-only root/subpath cases so it matches the complete
CommonJS matrix, and update all corresponding expectations to distinguish
resolver failures from fixture errors.
- Around line 471-480: Extend the test helper run in the NODE_PATH resolution
case to use multiple delimiter-separated directories, with the first containing
a package whose exports map blocks the requested subpath and the second
containing a valid matching package. Add assertions for the resolver’s resulting
behavior, including relevant empty or delimiter-only NODE_PATH forms if covered
by this test, so later entries remain eligible after the first returns NotFound.
🪄 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: 72cb67c9-3536-42fe-9191-20c4114e3c9e
📒 Files selected for processing (2)
src/resolver/resolver.rstest/js/bun/resolve/resolve.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/resolver/resolver.rs:3637— The extracted helper setsdir_info: Some(pkg_dir_info)here, but the deleted inline block setdir_info: Some(dir_info)— the outer node_modules-walk directory — and the un-extracted parallel copy at resolver.rs:3143 still does.pkg_dir_infois arguably the right value (it's what theNonefallback at :2338 andhandle_esm_resolution's own success path use), but since this refactor is described as a pure extraction it's worth calling the change out explicitly and aligning :3143 to match.Extended reasoning...
What changed
In the
esm.subpath == b"./package.json"fallback of the newly-extractedresolve_from_package_exports, theMatchResultis built withdir_info: Some(pkg_dir_info)— the package's own directory. The pre-refactor inlined block that this helper replaces setdir_info: Some(dir_info), wheredir_infoat that scope was the outer loop variable of the upward node_modules walk (an ancestor of the importer, not of the package). The un-refactored parallel copy of this same block at resolver.rs:3143 still uses the outerdir_info, so the two copies now diverge.Why it matters (a little)
MatchResult.dir_infohas exactly one consumer, at resolver.rs:2336: whencare_about_browser_fieldis set, it callsbase_dir_info.get_enclosing_browser_scope()for browser-field remapping. Starting the enclosing-scope search from the package directory vs. the importer's node_modules ancestor can, in principle, land on a differentbrowsermap.The PR description frames the extraction as behavior-preserving ("The node_modules walk now calls this helper in place of the inlined block"), and REVIEW.md flags exactly this class: "Treat every refactor as guilty until proven behavior-preserving. Diff the old path's complete behavior — out-parameter writes...". This is an out-parameter write that silently changed.
Step-by-step
- Before this PR, at the deleted hunk's scope,
dir_infois the loop variable ofwhile let Some(dir_info) = ...; dir_info = dir_info.get_parent()— an ancestor of the importing file. pkg_dir_infoisself.dir_info_cached(<dir>/node_modules/<name>)— the package's own directory.- Old code stored (1); new code stores (2). The parallel block at :3143 still stores (1).
- At :2336,
res.dir_infofeedsget_enclosing_browser_scope(). IfSome(pkg_dir_info)wereNone, the fallback at :2338 would re-derive fromresult.path_pair.primary.name().dir, which for this branch ispkg_dir_info— so the new value matches the fallback and matcheshandle_esm_resolution's own success path (which setsdir_infoto the resolved directory). The old value looks like the outlier.
Impact
Extremely narrow: this branch fires only for
require("pkg/package.json")wherepkghas an"exports"map that hides./package.json, undercare_about_browser_field, and browser-field remapping essentially never targets apackage.jsonfile. No demonstrated concrete failure.Suggested fix
Either note in the PR description that this out-parameter was intentionally corrected to
pkg_dir_infoand update the parallel copy at resolver.rs:3143 toSome(pkg_dir_info)for consistency, or revert this line to preserve exact prior behavior. The former seems right —pkg_dir_infois what every sibling path already uses. - Before this PR, at the deleted hunk's scope,
A NODE_PATH segment is arbitrary environment input. The unchecked join panics when the normalized path overflows the fixed PathBuffer, where the pre-existing legacy probe in the same loop already used the checked variant and skipped the entry.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/resolver/resolver.rs (1)
3516-3569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the duplicated
conditionsmatch out of the retry block.The
ESModule { conditions: match kind {...}, ... }block (line 3517-3526) is repeated verbatim at line 3553-3562 for the extension-stripped retry. Computeconditionsonce before the firstresolvecall and reuse it for both attempts.♻️ Proposed refactor
let mut module_type = options::ModuleType::Unknown; + let conditions = match kind { + ast::ImportKind::Require | ast::ImportKind::RequireResolve => { + &self.opts.conditions.require + } + ast::ImportKind::At | ast::ImportKind::AtConditional => &self.opts.conditions.style, + _ => &self.opts.conditions.import, + }; { let esm_resolution = ESModule { - conditions: match kind { - ast::ImportKind::Require | ast::ImportKind::RequireResolve => { - &self.opts.conditions.require - } - ast::ImportKind::At | ast::ImportKind::AtConditional => { - &self.opts.conditions.style - } - _ => &self.opts.conditions.import, - }, + conditions, debug_logs: self.debug_logs.as_mut(), module_type: &mut module_type, }(same substitution at the retry-without-extension block)
🤖 Prompt for 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. In `@src/resolver/resolver.rs` around lines 3516 - 3569, In the resolver flow containing the two ESModule constructions, compute the conditions reference from kind once before the initial resolve attempt, then reuse that variable in both ESModule instances, including the extension-stripped retry. Remove the duplicated conditions match while preserving the existing mappings for require, style, and import kinds.
♻️ Duplicate comments (3)
src/resolver/resolver.rs (3)
2687-2715: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
abs_buf_checkedfor the NODE_PATH ESM package-path join.Line 2688-2690 joins
path(aNODE_PATHentry) withesm.nameusing uncheckedabs_buf.abs_bufpanics when the joined path exceeds the buffer, whileabs_buf_checkedreturnsNoneinstead.NODE_PATHis an environment variable under user/deployment control and can hold an arbitrarily long value, so this join can crash the process on a longNODE_PATHentry or package name.The legacy fallback a few lines later (line 2716-2721) already uses
abs_buf_checkedand gracefullycontinues past the offending entry on overflow. Apply the same pattern to the ESM exports probe: skip only the exports lookup onNoneand fall through to the existing checked legacy path.This is the same concern raised in a prior review at this location; it does not appear to have been applied in the code shown here.
🛡️ Proposed fix
for path in node_path.split(|&b| b == delim).filter(|s| !s.is_empty()) { if let Some(ref esm) = esm_ { - let abs_package_path: &[u8] = self - .fs_ref() - .abs_buf(&[path, esm.name], bufs!(esm_absolute_package_path)); - if let Ok(Some(pkg_dir_info)) = self.dir_info_cached(abs_package_path) { + if let Some(abs_package_path) = self + .fs_ref() + .abs_buf_checked(&[path, esm.name], bufs!(esm_absolute_package_path)) + && let Ok(Some(pkg_dir_info)) = self.dir_info_cached(abs_package_path) + { let prev_extension_order = self.extension_order;As per path instructions, "User-reachable failures from input, syscalls, networks, files, CLI arguments, or environment variables must be catchable errors rather than panics."
🤖 Prompt for 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. In `@src/resolver/resolver.rs` around lines 2687 - 2715, Replace the unchecked abs_buf join in the ESM package-path probe with abs_buf_checked, handling None by skipping only the exports lookup and continuing to the existing checked legacy fallback. Preserve the current exports resolution, extension-order restoration, debug indentation, and return behavior when the path fits.Source: Path instructions
3502-3606: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resolve_from_package_exportsdiscards the resolved file's own module type.Line 3514 initializes
module_typefrom the top-level package'spackage_json.module_type, and lines 3544 and 3582 unconditionally overwriteout.module_typewith that local afterhandle_esm_resolutionsucceeds.
handle_esm_resolution(Status::Exact / ExactEndsWithStar branch, line 3772-3776 and 3793) already computes and writes the correctmodule_typeintoout, derived fromresolved_dir_info.package_json()— the package.json nearest to the actual resolved file, which can be a nested package with a different"type"than the top-level package. The subsequentout.module_type = module_type;in this function throws that value away and replaces it with the outer-scope value, which was never told about the nested package.A previous review flagged this exact pattern at this location and was marked "Addressed", but the code shown here still contains both overwrite assignments and the non-
Unknowninitialization as originally described. Re-verify whether the fix landed correctly for this helper.🐛 Proposed fix
- let mut module_type = package_json.module_type; + let mut module_type = options::ModuleType::Unknown;Then remove both overwrite assignments:
{ out.is_node_module = true; - out.module_type = module_type; return Some(MatchStatus::Success); }(applied at both success sites, line 3544 and line 3582)
🤖 Prompt for 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. In `@src/resolver/resolver.rs` around lines 3502 - 3606, In resolve_from_package_exports, remove both out.module_type = module_type assignments in the successful ES module resolution branches, allowing handle_esm_resolution to preserve the module type derived from the resolved file’s nearest package.json. Keep the existing success handling and module_type tracking otherwise unchanged.
3502-3530: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftThe auto-install exports path still lacks the
stylecondition, and CSS package-exports resolution has no regression test.
resolve_from_package_exportsappliesself.opts.conditions.styleforast::ImportKind::At/AtConditional(line 3522-3524). The other, still-inline exports-resolution block used for auto-installed/global-cache packages (lines 3030-3149, unchanged by this diff) only matchesRequire/RequireResolvevs. a catch-allimport— it never selectsconditions.style. A CSS@importresolving through an auto-installed package will therefore use the wrong condition set compared to the same package resolved from an existingnode_modules/NODE_PATHentry.Since this PR's stated goal is to centralize exports resolution into a shared helper, consider migrating the auto-install block to call
resolve_from_package_exportsas well, to avoid this divergence going forward.Separately, a prior review requested a regression test for CSS
@importthrough packageexportsdistinguishingstylefromimport/requireconditions; the provided test file does not appear to contain one yet.#!/bin/bash # Verify whether the auto-install exports block applies the CSS "style" condition # and whether a CSS `@import` package-exports test exists. rg -n -C 6 'conditions\.style' src/resolver/resolver.rs rg -n -C 6 'ImportKind::At' src/resolver/resolver.rs rg -n -i -C 10 '`@import`.*exports|exports.*style' test/js/bun/resolve/resolve.test.ts test/bundler🤖 Prompt for 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. In `@src/resolver/resolver.rs` around lines 3502 - 3530, Update the auto-install/global-cache package-exports resolution flow to reuse the shared resolve_from_package_exports method, ensuring CSS ImportKind::At and ImportKind::AtConditional use conditions.style consistently with regular package resolution. Add a regression test covering CSS `@import` through package exports with distinct style versus import/require conditions, verifying the style target is selected.
🤖 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.
Outside diff comments:
In `@src/resolver/resolver.rs`:
- Around line 3516-3569: In the resolver flow containing the two ESModule
constructions, compute the conditions reference from kind once before the
initial resolve attempt, then reuse that variable in both ESModule instances,
including the extension-stripped retry. Remove the duplicated conditions match
while preserving the existing mappings for require, style, and import kinds.
---
Duplicate comments:
In `@src/resolver/resolver.rs`:
- Around line 2687-2715: Replace the unchecked abs_buf join in the ESM
package-path probe with abs_buf_checked, handling None by skipping only the
exports lookup and continuing to the existing checked legacy fallback. Preserve
the current exports resolution, extension-order restoration, debug indentation,
and return behavior when the path fits.
- Around line 3502-3606: In resolve_from_package_exports, remove both
out.module_type = module_type assignments in the successful ES module resolution
branches, allowing handle_esm_resolution to preserve the module type derived
from the resolved file’s nearest package.json. Keep the existing success
handling and module_type tracking otherwise unchanged.
- Around line 3502-3530: Update the auto-install/global-cache package-exports
resolution flow to reuse the shared resolve_from_package_exports method,
ensuring CSS ImportKind::At and ImportKind::AtConditional use conditions.style
consistently with regular package resolution. Add a regression test covering CSS
`@import` through package exports with distinct style versus import/require
conditions, verifying the style target is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e7636c0-7a14-4441-b084-29271b1b0fbd
📒 Files selected for processing (1)
src/resolver/resolver.rs
There was a problem hiding this comment.
The abs_buf_checked fix in 922f78c addresses the panic I flagged; nothing new turned up on this revision. Deferring to a human because this is a refactor inside the core resolver plus a behaviour addition — worth eyes on the dir_info: Some(pkg_dir_info) change in the ./package.json fallback and the multi-entry NODE_PATH stop-on-NotFound semantics.
What was reviewed:
- Extracted
resolve_from_package_exportsdiffed against the removed inline block — byte-equivalent apart from the documenteddir_infofield change. - NODE_PATH loop:
abs_buf_checkedguard,extension_orderrestore on both arms,decrease_indent()paired with the singleincrease_indent()at the top ofload_node_modules. - Multi-entry NODE_PATH early-return checked against Node's
ERR_PACKAGE_PATH_NOT_EXPORTEDbehaviour (does not fall through to later entries).
Extended reasoning...
Overview
Two files: src/resolver/resolver.rs extracts the ~150-line inline package.json "exports" resolution block from the node_modules walk into a new resolve_from_package_exports() helper, and adds a second call site inside the NODE_PATH iteration loop so packages located via NODE_PATH get the same PACKAGE_EXPORTS_RESOLVE treatment as ones found in node_modules. test/js/bun/resolve/resolve.test.ts gains a subprocess-based test covering exports["."] vs main, mapped subpaths, unexported/null-blocked subpaths, scoped packages, a no-exports legacy package, and both CJS and ESM entry points.
Security risks
None identified. NODE_PATH is an env var the process owner controls; the change tightens encapsulation (blocks unexported files that previously loaded) rather than loosening it. The over-long-entry panic I raised earlier was fixed by switching to abs_buf_checked.
Level of scrutiny
High. resolver.rs is the module-resolution hot path for both the runtime and the bundler; a subtle regression here breaks every import. The extraction is mostly mechanical but carries one intentional semantic change (the ./package.json fallback's MatchResult.dir_info now points at the package directory rather than the outer search directory), and the new NODE_PATH call site introduces stop-on-NotFound semantics for multi-entry NODE_PATH. Both are argued in the PR description and match Node, but they're the kind of resolver-semantics decisions a maintainer should sign off on.
Other factors
All prior review threads are resolved: my abs_buf panic finding was fixed in 922f78c, the third inline copy in the auto-install path is intentionally left alone and documented in the PR description, and CodeRabbit's findings were withdrawn or addressed. The bug-hunting pass on the current revision found nothing. The new test is well-constructed (hermetic tempDir, drains both pipes, asserts stderr/stdout before exitCode, covers scoped + legacy fallback). The PR footer notes tests were deferred to CI rather than run locally, which is another reason to let CI + a human confirm before merge.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/resolver/resolver.rs (1)
3514-3514: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the resolved export target's module type.
Initialize
module_typetooptions::ModuleType::Unknownand remove bothout.module_type = module_typeassignments.handle_esm_resolutionalready writesout.module_typefrom the package that contains the resolved file. The current assignments overwrite a nested target package's module type with the outer package's"type"value. This can misclassify an ESM target as CJS, or the reverse.This repeats the previously reported issue in this helper.
Proposed fix
- let mut module_type = package_json.module_type; + let mut module_type = options::ModuleType::Unknown; ... - out.module_type = module_type; ... - out.module_type = module_type;Also applies to: 3543-3545, 3581-3583
🤖 Prompt for 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. In `@src/resolver/resolver.rs` at line 3514, In the resolver helper around module_type and handle_esm_resolution, initialize module_type as options::ModuleType::Unknown instead of package_json.module_type, and remove both assignments that write module_type to out.module_type. Preserve the module type already populated by handle_esm_resolution for the package containing the resolved target.
🤖 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.
Duplicate comments:
In `@src/resolver/resolver.rs`:
- Line 3514: In the resolver helper around module_type and
handle_esm_resolution, initialize module_type as options::ModuleType::Unknown
instead of package_json.module_type, and remove both assignments that write
module_type to out.module_type. Preserve the module type already populated by
handle_esm_resolution for the package containing the resolved target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f48da3fd-5177-451e-9e01-90299cf237aa
📒 Files selected for processing (1)
src/resolver/resolver.rs
There was a problem hiding this comment.
The earlier abs_buf panic I flagged is fixed in 922f78c, and the third-copy nit is acknowledged in the description. I didn't find anything new. Deferring to a human for the resolver-side sign-off: the extraction is nearly verbatim, but the ./package.json fallback now stores pkg_dir_info where the node_modules walk previously stored the outer dir_info — a small behavioral change to the existing path that's worth a maintainer glance.
What was reviewed:
- Confirmed the NODE_PATH probe now uses
abs_buf_checkedand skips on overflow, matching the sibling join below it. - Verified
resolve_from_package_exportsis a faithful move of the removed inline block (conditions match,.jsretry,./package.jsonfallback, terminalNotFound), and thatdecrease_indentat both call sites balances the singleincrease_indentat the top ofload_node_modules. - Checked the multi-entry
NODE_PATHearly-return semantics against Node (first entry with the package wins, encapsulation error stops the search). - Test fixture covers exports-vs-main, mapped subpath, unexported/null-blocked, scoped package, and no-exports fallback across CJS and ESM.
Extended reasoning...
Overview
This PR fixes NODE_PATH package resolution to honor package.json "exports", matching Node's behavior. It touches two files: src/resolver/resolver.rs (extracts ~150 lines from the node_modules walk into resolve_from_package_exports() and calls it from both the node_modules walk and a new NODE_PATH exports probe) and test/js/bun/resolve/resolve.test.ts (a comprehensive new test in the existing NODE_PATH test group).
Security risks
None identified. The change tightens encapsulation (unexported subpaths that previously loaded via NODE_PATH now fail), which is strictly safer. The user-controlled NODE_PATH env-var input is now joined with abs_buf_checked (fixed after my earlier finding), so over-long entries are skipped rather than panicking.
Level of scrutiny
High. resolver.rs is on the hot path of every import/require in every Bun program. The extraction is near-verbatim, but there is one intentional behavioral change to the pre-existing node_modules walk: the ./package.json-subpath fallback now sets MatchResult.dir_info to the package's own pkg_dir_info instead of the outer search directory's dir_info. The PR description justifies this as consistent with what handle_esm_resolution and the None fallback already store there, and it looks right to me, but a maintainer familiar with what downstream consumers read from MatchResult.dir_info should confirm — it's the one place the "faithful move" claim doesn't hold byte-for-byte.
Other factors
- All prior review threads (mine, CodeRabbit's, comment-cop's) are resolved. My blocking
abs_buffinding was fixed in 922f78c; the paragraph-comment warning was addressed by trimming to one-line comments; the third-inline-copy nit was intentionally deferred with rationale added to the PR description. - I checked that CodeRabbit's
out.module_type = module_typeconcern doesn't apply as a regression: the removed inline block did the exact same assignment, so the helper preserves existing node_modules-walk behavior. debug_logsindent balance is correct at both call sites (singleincrease_indentat line 2505,decrease_indentat every return including the two new ones).- Test coverage is solid: 8 CJS + 4 ESM probes, including a
legacy-onlypackage with no"exports"to guard the fallthrough, and a scoped package. The test spawns with--no-install, spreadsbunEnv, drains stdout/stderr concurrently, and asserts exit code last. - The PR description notes tests were deferred to CI ("Platform-specific test(s) that do not run on this machine"), so there's no local test-pass confirmation.
|
CI state: Correction (2026-10-30): this comment overstated the CI evidence. Build #86339 was canceled when a newer commit was pushed, and 2 of its test jobs did not finish. Build #86375 was still in progress when this comment was posted. It later finished with all 196 jobs passed. |
|
Closing this PR because it has been inactive for more than 90 days. |
|
Replaced by #47500, which is built on current main. That PR reads |
A package located via
NODE_PATHhas its"exports"field ignored:exports["."]loses to the legacy"main", mapped subpaths fail to resolve, and unexported /null-blocked files load. Node treats eachNODE_PATHentry the same way it treats anode_modulesdirectory and runsPACKAGE_EXPORTS_RESOLVEon the package found there.Repro
require("npkg")EXPORTS-DOT-MAINLEGACY-MAINEXPORTS-DOT-MAINrequire("npkg/mapped")REAL-TARGETMODULE_NOT_FOUNDREAL-TARGETrequire("npkg/hidden.js")ERR_PACKAGE_PATH_NOT_EXPORTEDHIDDEN-UNEXPORTEDMODULE_NOT_FOUNDrequire("npkg/priv/secret.js")ERR_PACKAGE_PATH_NOT_EXPORTEDPRIV-NULLBLOCKED-SECRETMODULE_NOT_FOUND(Bun already reports
MODULE_NOT_FOUNDrather thanERR_PACKAGE_PATH_NOT_EXPORTEDfor an unexported subpath reached throughnode_modules; that error-code difference is pre-existing and unchanged here.)This affects exports-using packages deployed through a
NODE_PATHchannel (for example Lambda layers withNODE_PATH=/opt/nodejs/node_modules, orNODE_PATH=./srcalias setups): they silently load the legacy"main"build as their entry and lose the private-subpath fence.Cause
load_node_modulesiteratesNODE_PATHentries by joining each one with the rawimport_pathand callingload_as_file_or_directory, which only consultsmain/index. The upwardnode_moduleswalk a few hundred lines earlier splits the specifier into{name, subpath}, opens<dir>/node_modules/<name>, and applies the package's"exports"map before ever touchingload_as_file_or_directory. TheNODE_PATHloop never reached that code.Fix
Extract the exports-map probe that was inlined in the
node_moduleswalk intoresolve_from_package_exports(esm, abs_package_path, pkg_dir_info, kind, out) -> Option<MatchStatus>:None: the directory has nopackage.jsonor no"exports"field; the caller falls through to the legacymain/index lookup.Some(Success): the exports map matched andoutis filled.Some(NotFound): the package has"exports"and the subpath is not exported (encapsulation).The
node_moduleswalk now calls this helper in place of the inlined block. TheNODE_PATHloop computesabs_package_path = <entry>/<pkg-name>for each entry, calls the helper, and only falls through to the existingload_as_file_or_directorywhen the helper returnsNone. Packages without an"exports"field resolve exactly as before.The third inline copy of this sequence in the auto-install / global-cache path (
'load_module_from_cache) is left as-is: it diverges from thenode_modulescopy in a few pre-existing ways (module_typeseed, nostyleconditions arm, noout.module_typewrite) that would need their own review, so migrating it here would change that path's behaviour. The./package.jsonfallback in the helper now storespkg_dir_info(the package directory) where the old inline block stored the outer search directory; that matches whathandle_esm_resolutionand theNonefallback already use for this field.Verification
New test in
test/js/bun/resolve/resolve.test.tsunder the existingNODE_PATH testgroup coversexports["."]vs"main", a mapped subpath, an unexported file, anull-blocked subtree, a scoped package, and a package with no"exports"field (legacy fallback must still work). The test fails onmainwith the behaviour shown above and passes with this change. ExistingNODE_PATHtests andtest/bundler/esbuild/packagejson.test.tspass unchanged.no test proof · iteration 1 · 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