fix(resolve): make tryNodeResolve more robust#9170
fix(resolve): make tryNodeResolve more robust#9170aleclarson wants to merge 12 commits intovitejs:mainfrom
Conversation
488e9ee to
02ddba9
Compare
| if (!pkg) { | ||
| return | ||
| } | ||
| const nodeModules = lookupNodeModules(basedir) |
There was a problem hiding this comment.
lookupNodeModules is an alias for the built-in Module._nodeModulePaths function
This is the same way Node.js gets the node_modules directories to check for dependencies in.
There was a problem hiding this comment.
One concern I have is that this function produces node_modules directories outside of the importer's root directory (which I would define as the directory with .git, although we might also want to consider SVN or Mercurial support too). It feels wrong for a bundler to allow node_modules access outside the root directory, as this isn't reproducible by default outside the current machine. Any opinions on this, @patak-dev?
There was a problem hiding this comment.
Maybe we should let the user access the folder they whitelisted for fs access? Even if they are out of the .git root?
| } | ||
| const index = tryFsResolve(file + '/index', options) | ||
| if (index) return index + postfix | ||
| const indexFile = tryIndexFile(file, targetWeb, options) |
There was a problem hiding this comment.
This change avoids checking for {file}/index/index or {file}/index/package.json, both of which would be incorrect if found.
| .join('/') | ||
|
|
||
| let basedir: string | ||
| if (dedupe?.some((id) => possiblePkgIds.includes(id))) { |
There was a problem hiding this comment.
No need for possiblePkgIds anymore with the new logic, as long we assume the resolve.dedupe array only ever references package IDs and not module IDs. This assumption makes sense to me!
905d9cc to
2b7b452
Compare
Closes vitejs#4439 More context: vitejs#10504
Bug introduced in vitejs#10683 Some packages use "require" instead of "default" for CJS entry
…in the `getInlineConditions` function.
…and avoid breaking certain `tryFsResolve` calls too (like with `es5-ext`)
The "{id}/package.json" lookup done by `resolvePackageData` is insufficient for certain edge cases, like when "node_modules/{dep}" is linked to a directory without a package.json in it. With this PR, you can now import any file from node_modules even if it has no package.json file associated with it. This mirrors the same capability in Node's resolution algorithm.
In addition to supporting more edge cases, this new implementation might also be faster in some cases, since we are doing less lookups than compared to the previous behavior of calling `resolvePackageData` for every path in the `possiblePkgIds` array.
85d90b1 to
f62843d
Compare
|
Just rebased onto |
…instead of only the resolvedPkgId variable
e303e99 to
fff33dc
Compare
|
|
||
| const workspaceRootFiles = ['lerna.json', 'pnpm-workspace.yaml', '.git'] | ||
|
|
||
| export function isWorkspaceRoot( |
There was a problem hiding this comment.
Instead of using searchForWorkspaceRoot, I added this function, which takes advantage of the packageCache. It also only checks the given directory, instead of an upward traversal.
|
We've recently reworked I think it'll be easier to open a new PR for this if there's interest in it. Closing for now. |
Description
The
"{id}/package.json"lookup done byresolvePackageDatais insufficient for certain edge cases, like when"node_modules/{dep}"is linked to a directory without a package.json in it. With this PR, you can now import any file from node_modules even if it has no package.json file associated with it. This mirrors the same capability in Node's resolution algorithm.Probably fixes #6061 as reported by @sodatea here, since the
getPossibleModuleIdsfunction now includes the full module ID (even if it has an extension) in the returned array.Additional context
Help wanted with testing...Here's a (non-exhaustive?) list of cases that should be supported now:
foo/barwhennode_modules/foo/bar.jsexists without package.jsonxyzwhennode_modules/xyz.jsexistsxyzwhennode_modules/xyz/index.jsexists without package.jsonedit: Tests added!
What is the purpose of this pull request?