esm: fix hint on invalid module specifier

nodejs-github-bot · Dec 23, 2023 · Dec 19, 2023 · Dec 19, 2023 · Dec 19, 2023 · Dec 20, 2023
commit 1726a569e232e363558ad7da66bfce449defd78b
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -3,11 +3,10 @@
 const {
   ArrayIsArray,
   ArrayPrototypeJoin,
-  ArrayPrototypeShift,
+  ArrayPrototypeMap,
   JSONStringify,
   ObjectGetOwnPropertyNames,
   ObjectPrototypeHasOwnProperty,
-  RegExp,
   RegExpPrototypeExec,
   RegExpPrototypeSymbolReplace,
   SafeMap,
@@ -21,6 +20,7 @@ const {
   StringPrototypeSlice,
   StringPrototypeSplit,
   StringPrototypeStartsWith,
+  encodeURIComponent,
 } = primordials;
 const internalFS = require('internal/fs/utils');
 const { BuiltinModule } = require('internal/bootstrap/realm');
@@ -30,7 +30,7 @@ const { getOptionValue } = require('internal/options');
 const policy = getOptionValue('--experimental-policy') ?
   require('internal/process/policy') :
   null;
-const { sep, relative, toNamespacedPath, resolve } = require('path');
+const { sep, posix: { relative: relativePosixPath }, toNamespacedPath, resolve } = require('path');
 const preserveSymlinks = getOptionValue('--preserve-symlinks');
 const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
 const experimentalNetworkImports =
@@ -924,29 +924,34 @@ function resolveAsCommonJS(specifier, parentURL) {
     // If it is a relative specifier return the relative path
     // to the parent
     if (isRelativeSpecifier(specifier)) {
-      found = relative(parent, found);
-      // Add '.separator if the path does not start with '..separator'
+      const foundURL = pathToFileurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Fnodejs%2Fnode%2Fpull%2F51223%2Fcommits%2Ffound).pathname;
+      found = relativePosixPath(
+        StringPrototypeSlice(parentURL, 'file://'.length, StringPrototypeLastIndexOf(parentURL, '/')),
+        foundURL);
+
+      // Add './' if the path does not start with '../'
       // This should be a safe assumption because when loading
       // esm modules there should be always a file specified so
       // there should not be a specifier like '..' or '.'
-      if (!StringPrototypeStartsWith(found, `..${sep}`)) {
-        found = `.${sep}${found}`;
+      if (!StringPrototypeStartsWith(found, '../')) {
+        found = `./${found}`;
       }
     } else if (isBareSpecifier(specifier)) {
       // If it is a bare specifier return the relative path within the
       // module
-      const pkg = StringPrototypeSplit(specifier, '/')[0];
-      const index = StringPrototypeIndexOf(found, pkg);
+      const pkg = StringPrototypeSlice(specifier, 0, StringPrototypeIndexOf(specifier, '/'));
+      const index = StringPrototypeIndexOf(found, `node_modules${sep}${pkg}${sep}`);
       if (index !== -1) {
-        found = StringPrototypeSlice(found, index);
+        found = ArrayPrototypeJoin(
+          ArrayPrototypeMap(
+            StringPrototypeSplit(StringPrototypeSlice(found, index + `node_modules${sep}`.length), sep),
+            // Escape URL-special characters to avoid generating a incorrect suggestion
+            encodeURIComponent,
+          ),
+          '/',
+        );
       }
     }
-    // Normalize the path separator to give a valid suggestion
-    // on Windows
-    if (process.platform === 'win32') {
-      found = RegExpPrototypeSymbolReplace(new RegExp(`\\${sep}`, 'g'),
-                                           found, '/');
-    }
     return found;
   } catch {
     return false;
@@ -1154,14 +1159,14 @@ function defaultResolve(specifier, context = {}) {
  */
 function decorateErrorWithCommonJSHints(error, specifier, parentURL) {
   const found = resolveAsCommonJS(specifier, parentURL);
-  if (found) {
+  if (found && found !== specifier) {
     // Modify the stack and message string to include the hint
-    const lines = StringPrototypeSplit(error.stack, '\n');
+    const endOfFirstLine = StringPrototypeIndexOf(error.stack, '\n');
     const hint = `Did you mean to import ${found}?`;
     error.stack =
-      ArrayPrototypeShift(lines) + '\n' +
-      hint + '\n' +
-      ArrayPrototypeJoin(lines, '\n');
+      StringPrototypeSlice(error.stack, 0, endOfFirstLine) + '\n' +
+      hint +
+      StringPrototypeSlice(error.stack, endOfFirstLine);
     error.message += `\n${hint}`;
   }
 }

diff --git a/test/es-module/test-esm-module-not-found-commonjs-hint.mjs b/test/es-module/test-esm-module-not-found-commonjs-hint.mjs
@@ -1,32 +1,49 @@
 import { spawnPromisified } from '../common/index.mjs';
-import { fixturesDir } from '../common/fixtures.mjs';
+import { fixturesDir, fileURL as fixtureSubDir } from '../common/fixtures.mjs';
 import { match, notStrictEqual } from 'node:assert';
 import { execPath } from 'node:process';
 import { describe, it } from 'node:test';
 
 
 describe('ESM: module not found hint', { concurrency: true }, () => {
   for (
-    const { input, expected }
+    const { input, expected, cwd = fixturesDir }
     of [
       {
         input: 'import "./print-error-message"',
-        // Did you mean to import ../print-error-message.js?
-        expected: / \.\.\/print-error-message\.js\?/,
+        // Did you mean to import ./print-error-message.js?
+        expected: / \.\/print-error-message\.js\?/,
+      },
+      {
+        input: 'import "./es-modules/folder%25with percentage?/index.js"',
+        // Did you mean to import ./es-modules/name%2525with%20percentage%3F/index.js?
+        expected: / \.\/es-modules\/folder%2525with%20percentage%3F\/index\.js\?/,
+      },
+      {
+        input: 'import "../folder%25with percentage?/index.js"',
+        // Did you mean to import ../es-modules/name%2525with%20percentage%3F/index.js?
+        expected: / \.\.\/folder%2525with%20percentage%3F\/index\.js\?/,
+        cwd: fixtureSubDir('es-modules/tla/'),
       },
       {
         input: 'import obj from "some_module/obj"',
         expected: / some_module\/obj\.js\?/,
       },
+      {
+        input: 'import obj from "some_module/folder%25with percentage?/index.js"',
+        expected: / some_module\/folder%2525with%20percentage%3F\/index\.js\?/,
+      },
+      {
+        input: 'import obj from "lone_file.js"',
+        expected: /^(?!.*Did you mean to import lone_file\.js).*$/,
+      },
     ]
   ) it('should cite a variant form', async () => {
     const { code, stderr } = await spawnPromisified(execPath, [
       '--input-type=module',
       '--eval',
       input,
-    ], {
-      cwd: fixturesDir,
-    });
+    ], { cwd });
 
     match(stderr, expected);
     notStrictEqual(code, 0);

diff --git a/test/fixtures/es-modules/folder%25with percentage?/index.js b/test/fixtures/es-modules/folder%25with percentage?/index.js
@@ -0,0 +1 @@
+'use strict';
diff --git a/test/fixtures/node_modules/lone_file.js b/test/fixtures/node_modules/lone_file.js
diff --git a/test/fixtures/node_modules/some_module/folder%25with percentage?/index.js b/test/fixtures/node_modules/some_module/folder%25with percentage?/index.js