fix(resolve): make tryNodeResolve more robust #9170

aleclarson · 2022-07-17T16:26:03Z

⚠️ Merge #10504 before this

Description

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.

Probably fixes #6061 as reported by @sodatea here, since the getPossibleModuleIds function 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:

import foo/bar when node_modules/foo/bar.js exists without package.json
import xyz when node_modules/xyz.js exists
import xyz when node_modules/xyz/index.js exists without package.json

edit: Tests added!

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

aleclarson · 2022-07-17T16:34:53Z

packages/vite/src/node/plugins/resolve.ts

-  if (!pkg) {
-    return
-  }
+  const nodeModules = lookupNodeModules(basedir)


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.

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?

Maybe we should let the user access the folder they whitelisted for fs access? Even if they are out of the .git root?

Continued here: #9170 (comment)

packages/vite/src/node/plugins/resolve.ts

aleclarson · 2022-07-17T17:13:52Z

packages/vite/src/node/plugins/resolve.ts

@@ -506,8 +512,10 @@ function tryResolveFile(
          }
        }
      }
-      const index = tryFsResolve(file + '/index', options)
-      if (index) return index + postfix
+      const indexFile = tryIndexFile(file, targetWeb, options)


This change avoids checking for {file}/index/index or {file}/index/package.json, both of which would be incorrect if found.

aleclarson · 2022-07-17T17:37:45Z

packages/vite/src/node/plugins/resolve.ts


  let basedir: string
-  if (dedupe?.some((id) => possiblePkgIds.includes(id))) {


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!

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.

aleclarson · 2022-11-14T20:39:33Z

Just rebased onto main

…instead of only the resolvedPkgId variable

aleclarson · 2022-11-14T22:45:10Z

packages/vite/src/node/packages.ts

+
+const workspaceRootFiles = ['lerna.json', 'pnpm-workspace.yaml', '.git']
+
+export function isWorkspaceRoot(


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.

bluwy · 2023-04-01T09:43:49Z

We've recently reworked tryNodeResolve and tryFsResolve for Vite 4.3, and it changes a lot of what's done here. I think we have a more robust resolver now, but the test case here and "node_modules/{dep}" is linked to a directory without a package.json in it could be interesting to support still.

I think it'll be easier to open a new PR for this if there's interest in it. Closing for now.

aleclarson force-pushed the fix/flexible-node-resolve branch from 488e9ee to 02ddba9 Compare July 17, 2022 17:08

aleclarson commented Jul 17, 2022

View reviewed changes

aleclarson force-pushed the fix/flexible-node-resolve branch from 905d9cc to 2b7b452 Compare July 17, 2022 17:39

sapphi-red mentioned this pull request Jul 21, 2022

Error: Module "events" has been externalized for browser compatibility #9238

Closed

7 tasks

aleclarson added 7 commits November 14, 2022 12:33

feat(resolve): support "fallback array" in package exports field

165cdef

Closes vitejs#4439 More context: vitejs#10504

refactor: overrideConditions

606f126

fix: add module condition by default

a94200f

fix: config bundling regression

296fe46

Bug introduced in vitejs#10683 Some packages use "require" instead of "default" for CJS entry

fix: respect overrideConditions and isRequire

4d3c6ac

…in the `getInlineConditions` function.

fix: try resolveExports call with postfix included

bb4fbd0

…and avoid breaking certain `tryFsResolve` calls too (like with `es5-ext`)

aleclarson force-pushed the fix/flexible-node-resolve branch from 85d90b1 to f62843d Compare November 14, 2022 20:39

aleclarson requested review from bluwy and sapphi-red and removed request for bluwy November 14, 2022 20:40

aleclarson added the p4-important Violate documented behavior or significantly improves performance (priority) label Nov 14, 2022

aleclarson added 4 commits November 14, 2022 17:09

test: unpackaged modules in node_modules

28c2587

fix: reset all resolvedPkg variables

173ef76

…instead of only the resolvedPkgId variable

fix: bugs caught by tests

5115d77

fix: stop looking in node_modules once a workspace root is found

fff33dc

aleclarson force-pushed the fix/flexible-node-resolve branch from e303e99 to fff33dc Compare November 14, 2022 22:43

aleclarson commented Nov 14, 2022

View reviewed changes

aleclarson mentioned this pull request Nov 14, 2022

feat(resolve): support subpath imports #10929

Closed

4 tasks

chore: fix lint

775ef0b

alex2600 mentioned this pull request Dec 1, 2022

Production build fails in runtime after update to 2.7.1 #6061

Closed

7 tasks

bluwy closed this Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolve): make tryNodeResolve more robust #9170

fix(resolve): make tryNodeResolve more robust #9170

aleclarson commented Jul 17, 2022 •

edited

aleclarson Jul 17, 2022

aleclarson Nov 14, 2022

patak-dev Nov 14, 2022

aleclarson Nov 14, 2022

aleclarson Jul 17, 2022 •

edited

aleclarson Jul 17, 2022

aleclarson commented Nov 14, 2022

aleclarson Nov 14, 2022 •

edited

bluwy commented Apr 1, 2023


		let basedir: string
		if (dedupe?.some((id) => possiblePkgIds.includes(id))) {


		const workspaceRootFiles = ['lerna.json', 'pnpm-workspace.yaml', '.git']

		export function isWorkspaceRoot(

fix(resolve): make tryNodeResolve more robust #9170

fix(resolve): make tryNodeResolve more robust #9170

Conversation

aleclarson commented Jul 17, 2022 • edited

⚠️ Merge #10504 before this

Description

Additional context

What is the purpose of this pull request?

aleclarson Jul 17, 2022

Choose a reason for hiding this comment

aleclarson Nov 14, 2022

Choose a reason for hiding this comment

patak-dev Nov 14, 2022

Choose a reason for hiding this comment

aleclarson Nov 14, 2022

Choose a reason for hiding this comment

aleclarson Jul 17, 2022 • edited

Choose a reason for hiding this comment

aleclarson Jul 17, 2022

Choose a reason for hiding this comment

aleclarson commented Nov 14, 2022

aleclarson Nov 14, 2022 • edited

Choose a reason for hiding this comment

bluwy commented Apr 1, 2023

aleclarson commented Jul 17, 2022 •

edited

aleclarson Jul 17, 2022 •

edited

aleclarson Nov 14, 2022 •

edited