Repository navigation
resolver: read package.json exports for a package found through NODE_PATH
#47500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
4
commits into
main
Choose a base branch
from
robobun/a7d10699/node-path-exports-step
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
c857ed3
resolver: read package.json "exports" for a package found through NOD…
robobun fa8fbbc
AOT: try only the exact "exports" keys of a package in NODE_PATH
robobun b95d4a6
test: run the three NODE_PATH "exports" tests of resolve.test.ts conc…
robobun bb99194
Merge remote-tracking branch 'origin/main' into robobun/a7d10699/node…
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) Projects whose NODE_PATH contains an entry under a regular file (the pnpm shape resolver.rs:4833 was written for) now pay two more failed open(2) calls on every resolution that reaches NODE_PATH, on top of the one the base paid. load_node_path_package_exports at src/resolver/resolver.rs:3881-3896 calls dir_info_cached on the entry and on
<entry>/<name>, and dir_info_cached_miss returns Ok(None) on ENOTDIR at resolver.rs:4836-4840 without recording the miss, so the helper's probes repeat on every call. Fix: remember ENOTDIR for the entry (cache the not-found like ENOENT at :4867), or probe dir_info_cached(node_path) once per entry and skip both the helper and the plain lookup when it is None.Why this was flagged
Trigger: NODE_PATH holds an entry whose ancestor is a file (the pnpm-generated list named in the comment at src/resolver/resolver.rs:4829-4835) and the project resolves bare specifiers that miss the node_modules walk. The new helper at resolver.rs:3881-3896 does dir_info_cached(dirname(abs_package_path)) and dir_info_cached(abs_package_path) before the old lookup at :3095. Each walk reaches the file ancestor, open_dir fails with ENOTDIR, and resolver.rs:4836-4840 returns Ok(None) before the mark_not_found at :4867, so nothing is cached and the next resolution opens again. The base did the same two uncached opens once in load_as_file_or_directory; the change doubles them to four per NODE_PATH entry per resolution, in both the runtime and the bundler, for every import that misses node_modules. Remedy: cache the ENOTDIR miss, or check the entry once per loop iteration and skip both lookups.
Verification: Triggered when NODE_PATH holds an entry whose ancestor is a regular file and a bare specifier misses the node_modules walk. In
dir_info_cached_miss, an open that fails with ENOTDIR/EISDIR doesreturn Ok(None)at src/resolver/resolver.rs:4836-4840 before themark_not_foundcalls at :4867-4868, so the next call re-opens the same path. So the PR adds 2 uncached failed opens per such entry per miss.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not changed in this PR. The count is right: for an entry that is a regular file, or lies below one, one warm call makes 3 failed
openatwhere main makes 1. The PR body has it under Measurements. It applies to a bare specifier that nonode_moduleshas.I checked both proposals.
dir_info_cached_miss. This is the root: the arm returns before the twomark_not_foundcalls. The change also removes the one open that main makes, and it makes theis_listed_fileguard of the helper redundant. But it changes the negative cache for every caller of the resolver, so it is not a change for this PR.dir_info_cached(entry)isNone.Noneis also the answer for a folder that cannot be listed. Withchmod 311on the entry and a package below it, as a user that is not root: main loadslegacy.jsby"main", and this PR loadsm.jsby"exports". With the skip, the package is not found.The helper cannot tell ENOTDIR from EACCES on the parent, because
dir_info_cachedanswersOk(None)for both.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up on the first proposal (remember ENOTDIR in
dir_info_cached_miss). I built it and measured it. I do not send it as a PR.What it gives. With
NODE_PATHset to a file, 1000 failed lookups of 3 names make 4 failed opens (main 34cbc70: 2001).What it costs. A path that was a file stays "not found" after it becomes a directory, until
bust_dir_cachedrops that exact path. Measured against main:import "late"builds whilenode_modules/lateis a file. Afterlatebecomes a package folder, each laterBun.build()in the process fails withCould not resolve: "late". Main builds.require.resolve("./was/dir")fails once whilewasis a file. Afterwasbecomes a directory, main resolves it. A path that was missing at first also resolves. The change givesMODULE_NOT_FOUND.import "./sub/x"whilesubis a file, thenBun.build()again aftersubbecomes a directory: 0 of 4 shapes build (a file or a directory import, targetbunorbrowser). Main builds 3 of 4.node_modulesabove them. With auto-install on,requireloadedlatestfrom the registry. On main the installed version loads.I found no issue that reports the repeated open. How long a cached "not found" may live is the subject of #40279 and #46805, and ENOTDIR belongs in that decision.
An option for this PR that adds no cache entry.
RealFS::read_directoryalready remembers ENOTDIR for the entry (src/resolver/lib.rs:1046) and tells it from EACCES. I applied this head on main 34cbc70 and made the helper ask that listing in place ofdir_info_cached(parent):chmod 311entry still resolvesm.js, as a user that is not root.test/js/bun/resolve/resolve.test.tsof this PR passes (111 pass). I did not run the bundler, AOT and Rust tests."main", as on main. This head reads"exports"there.Patch for the helper
Two test rows: an entry that is a file, or lies below one, is skipped (they pass on main, on this head, and with the patch)