Improve node_modules resolver (#219)

Author: calebeby

.changeset/red-moles-leave.md

@@ -0,0 +1,5 @@
+---
+'pleasantest': minor
+---
+
+Improve node_modules resolver (now it is able to resolve multiple package versions and it supports pnpm)

package-lock.json

@@ -7,7 +7,7 @@ import * as esbuild from 'esbuild';
 import { parse } from 'cjs-module-lexer';
 // @ts-expect-error @types/node@12 doesn't like this import
 import { createRequire } from 'module';
-import { isBareImport, npmPrefix } from './extensions-and-detection';
+import { isBareImport } from './extensions-and-detection';
 let npmCache: RollupCache | undefined;
 
 /**
@@ -110,8 +110,6 @@ export { default } from '${mod}'`;
 const pluginNodeResolve = (): Plugin => ({
   name: 'node-resolve',
   resolveId(id) {
-    if (isBareImport(id)) return { id: npmPrefix + id, external: true };
-    // If requests already have the npm prefix, mark them as external
-    if (id.startsWith(npmPrefix)) return { id, external: true };
+    if (isBareImport(id)) return { id, external: true };
   },
 });

src/module-server/bundle-npm-module.ts

@@ -67,30 +67,30 @@ export const jsMiddleware = async ({
     try {
       // Normalized path starting with slash
       const path = posix.normalize(req.path);
+      const params = new URLSearchParams(req.query as Record<string, string>);
       let id: string;
       let file: string;
       if (path.startsWith('/@npm/')) {
         id = path.slice(1); // Remove leading slash
-        file = ''; // This should never be read
+        file = params.get('resolvedPath') || '';
       } else {
         // Remove leading slash, and convert slashes to os-specific slashes
         const osPath = path.slice(1).split(posix.sep).join(sep);
         // Absolute file path
         file = resolve(root, osPath);
         // Rollup-style Unix-normalized path "id":
         id = file.split(sep).join(posix.sep);
+      }
 
-        const params = new URLSearchParams(req.query as Record<string, string>);
-        params.delete('import');
-        params.delete('inline-code');
-        params.delete('build-id');
+      params.delete('import');
+      params.delete('inline-code');
+      params.delete('build-id');
 
-        // Remove trailing =
-        // This is necessary for rollup-plugin-vue, which ads ?lang.ts at the end of the id,
-        // so the file gets processed by other transformers
-        const qs = params.toString().replace(/=$/, '');
-        if (qs) id += `?${qs}`;
-      }
+      // Remove trailing =
+      // This is necessary for rollup-plugin-vue, which ads ?lang.ts at the end of the id,
+      // so the file gets processed by other transformers
+      const qs = params.toString().replace(/=$/, '');
+      if (qs) id += `?${qs}`;
 
       res.setHeader('Content-Type', 'application/javascript;charset=utf-8');
       const resolved = await rollupPlugins.resolveId(id);

src/module-server/middleware/js.ts

@@ -20,7 +20,9 @@ afterAll(async () => {
 });
 
 const createFs = async (input: string) => {
-  const dir = await fs.mkdtemp(join(tmpdir(), 'pleasantest-create-fs-'));
+  const dir = await fs.realpath(
+    await fs.mkdtemp(join(tmpdir(), 'pleasantest-create-fs-')),
+  );
   createdPaths.push(dir);
   const paths = input
     .trim()
@@ -169,6 +171,30 @@ describe('resolving in node_modules', () => {
       './node_modules/preact/dist/hooks/index.js',
     );
   });
+
+  test('resolves multiple versions of a package correctly', async () => {
+    // A and B depend on different versions of C
+    // So they each have a different copy of C in their node_modules
+    const fs = await createFs(`
+      ./node_modules/a/package.json {}
+      ./node_modules/a/index.js
+      ./node_modules/a/node_modules/c/package.json {}
+      ./node_modules/a/node_modules/c/index.js
+      ./node_modules/b/package.json {}
+      ./node_modules/b/index.js
+      ./node_modules/b/node_modules/c/package.json {}
+      ./node_modules/b/node_modules/c/index.js
+      ./node_modules/c/package.json {}
+      ./node_modules/c/index.js {}
+    `);
+    expect(await fs.resolve('c', { from: './node_modules/b/index.js' })).toBe(
+      './node_modules/b/node_modules/c/index.js',
+    );
+    expect(await fs.resolve('c', { from: './node_modules/a/index.js' })).toBe(
+      './node_modules/a/node_modules/c/index.js',
+    );
+    expect(await fs.resolve('c')).toBe('./node_modules/c/index.js');
+  });
 });
 
 describe('resolving relative paths', () => {

src/module-server/node-resolve.test.ts

@@ -1,4 +1,4 @@
-import { dirname, join, posix, resolve as pResolve } from 'path';
+import { dirname, join, posix, relative, resolve as pResolve } from 'path';
 import { promises as fs } from 'fs';
 import { resolve, legacy as resolveLegacy } from 'resolve.exports';
 import {
@@ -9,7 +9,11 @@ import {
 // Only used for node_modules
 const resolveCache = new Map<string, ResolveResult>();
 
-const resolveCacheKey = (id: string, root: string) => `${id}\0\0${root}`;
+const resolveCacheKey = (
+  id: string,
+  importer: string | undefined,
+  root: string,
+) => `${id}\n${importer}\n${root}`;
 
 /**
  * Attempts to implement a combination of:
@@ -22,7 +26,7 @@ export const nodeResolve = async (
   importer: string,
   root: string,
 ) => {
-  if (isBareImport(id)) return resolveFromNodeModules(id, root);
+  if (isBareImport(id)) return resolveFromNodeModules(id, importer, root);
   if (isRelativeOrAbsoluteImport(id))
     return resolveRelativeOrAbsolute(id, importer);
 };
@@ -108,9 +112,10 @@ interface ResolveResult {
 
 export const resolveFromNodeModules = async (
   id: string,
+  importer: string | undefined,
   root: string,
 ): Promise<ResolveResult> => {
-  const cacheKey = resolveCacheKey(id, root);
+  const cacheKey = resolveCacheKey(id, importer, root);
   const cached = resolveCache.get(cacheKey);
   if (cached) return cached;
   const pathChunks = id.split(posix.sep);
@@ -120,10 +125,29 @@ export const resolveFromNodeModules = async (
   // Path within imported module
   const subPath = join(...pathChunks.slice(isNpmNamespace ? 2 : 1));
 
-  const pkgDir = join(root, 'node_modules', ...packageName);
-  const stats = await stat(pkgDir);
-  if (!stats || !stats.isDirectory())
-    throw new Error(`Could not find ${id} in node_modules`);
+  const realRoot = await fs.realpath(root).catch(() => root);
+
+  // Walk up folder by folder until a folder is found with <folder>/node_modules/<pkgName>
+  // i.e. for 'asdf' from a/b/c.js look at
+  // a/b/node_modules/asdf,
+  // a/node_modules/asdf,
+  // node_modules/asdf,
+
+  let pkgDir: string | undefined;
+  let scanDir = importer
+    ? await fs
+        .realpath(importer)
+        .then((realImporter) => relative(realRoot, realImporter))
+        .catch(() => relative(root, importer))
+    : '.';
+  while (!pkgDir || !(await stat(pkgDir))?.isDirectory()) {
+    if (scanDir === '.' || scanDir.startsWith('..')) {
+      throw new Error(`Could not find ${id} in node_modules`);
+    }
+    // Not found; go up a level and try again
+    scanDir = dirname(scanDir);
+    pkgDir = join(root, scanDir, 'node_modules', ...packageName);
+  }
 
   const pkgJson = await readPkgJson(pkgDir);
   const main = readMainFields(pkgJson, subPath, true);
@@ -144,7 +168,10 @@ export const resolveFromNodeModules = async (
       version ? `${normalizedPkgName}@${version}` : normalizedPkgName,
       subPath,
     );
-    const resolved: ResolveResult = { path: result, idWithVersion };
+    const resolved: ResolveResult = {
+      path: await fs.realpath(result),
+      idWithVersion,
+    };
     resolveCache.set(cacheKey, resolved);
     return resolved;
   }

src/module-server/node-resolve.ts

@@ -42,37 +42,50 @@ export const npmPlugin = ({
   // Rewrite bare imports to have @npm/ prefix
   async resolveId(id, importer) {
     if (!isBareImport(id)) return;
-    const resolved = await resolveFromNodeModules(id, root).catch((error) => {
-      throw importer
-        ? changeErrorMessage(error, (msg) => `${msg} (imported by ${importer})`)
-        : error;
-    });
+    const resolved = await resolveFromNodeModules(id, importer, root).catch(
+      (error) => {
+        throw importer
+          ? changeErrorMessage(
+              error,
+              (msg) => `${msg} (imported by ${importer})`,
+            )
+          : error;
+      },
+    );
     if (!jsExts.test(resolved.path))
       // Don't pre-bundle, use the full path to the file in node_modules
       // (ex: CSS files in node_modules)
       return resolved.path;
 
-    return npmPrefix + id;
+    return `${npmPrefix}${id}?resolvedPath=${encodeURIComponent(
+      resolved.path,
+    )}&idWithVersion=${encodeURIComponent(resolved.idWithVersion)}`;
   },
-  async load(id) {
-    if (!id.startsWith(npmPrefix)) return;
-    id = id.slice(npmPrefix.length);
-    const resolved = await resolveFromNodeModules(id, root);
+  async load(fullId) {
+    if (!fullId.startsWith(npmPrefix)) return;
+    fullId = fullId.slice(npmPrefix.length);
+    const params = new URLSearchParams(fullId.slice(fullId.indexOf('?')));
+    const [id] = fullId.split('?');
+    let resolvedPath = params.get('resolvedPath');
+    let idWithVersion = params.get('idWithVersion');
+    if (!resolvedPath || !idWithVersion) {
+      const resolveResult = await resolveFromNodeModules(id, undefined, root);
+      resolvedPath = resolveResult.path;
+      idWithVersion = resolveResult.idWithVersion;
+    }
 
     const cachePath = join(
       cacheDir,
       '@npm',
-      `${resolved.idWithVersion}-${hash(envVars)}.js`,
+      `${idWithVersion}-${hash(envVars)}.js`,
     );
     const cached = await getFromCache(cachePath);
     if (cached) return cached;
-    const result = await bundleNpmModule(resolved.path, id, false, envVars);
+    const result = await bundleNpmModule(resolvedPath, id, false, envVars);
     // Queue up a second-pass optimized/minified build
-    bundleNpmModule(resolved.path, id, true, envVars).then(
-      (optimizedResult) => {
-        setInCache(cachePath, optimizedResult);
-      },
-    );
+    bundleNpmModule(resolvedPath, id, true, envVars).then((optimizedResult) => {
+      setInCache(cachePath, optimizedResult);
+    });
     setInCache(cachePath, result);
     return result;
   },