esm: fix hint on invalid module specifier

PR-URL: #51223
Fixes: #51216
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: aduh95

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 =
@@ -921,6 +921,7 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) {
  * Try to resolve an import as a CommonJS module.
  * @param {string} specifier - The specifier to resolve.
  * @param {string} parentURL - The base URL.
+ * @returns {string | Buffer | false}
  */
 function resolveAsCommonJS(specifier, parentURL) {
   try {
@@ -933,29 +934,38 @@ 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(found).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 i = StringPrototypeIndexOf(specifier, '/');
+      const pkg = i === -1 ? specifier : StringPrototypeSlice(specifier, 0, i);
+      const needle = `${sep}node_modules${sep}${pkg}${sep}`;
+      const index = StringPrototypeLastIndexOf(found, needle);
       if (index !== -1) {
-        found = StringPrototypeSlice(found, index);
+        found = pkg + '/' + ArrayPrototypeJoin(
+          ArrayPrototypeMap(
+            StringPrototypeSplit(StringPrototypeSlice(found, index + needle.length), sep),
+            // Escape URL-special characters to avoid generating a incorrect suggestion
+            encodeURIComponent,
+          ),
+          '/',
+        );
+      } else {
+        found = `${pathToFileURL(found)}`;
       }
     }
-    // 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;
@@ -1163,14 +1173,14 @@ function defaultResolve(specifier, context = {}) {
  */
 function decorateErrorWithCommonJSHints(error, specifier, parentURL) {
   const found = resolveAsCommonJS(specifier, parentURL);
-  if (found) {
+  if (found && found !== specifier) { // Don't suggest the same input the user provided.
     // Modify the stack and message string to include the hint
-    const lines = StringPrototypeSplit(error.stack, '\n');
-    const hint = `Did you mean to import ${found}?`;
+    const endOfFirstLine = StringPrototypeIndexOf(error.stack, '\n');
+    const hint = `Did you mean to import ${JSONStringify(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}`;
   }
 }

test/es-module/test-esm-module-not-found-commonjs-hint.mjs

@@ -1,32 +1,53 @@
 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/folder%2525with%20percentage%23/index.js"?
+        expected: / "\.\/es-modules\/folder%2525with%20percentage%23\/index\.js"\?/,
+      },
+      {
+        input: 'import "../folder%25with percentage#/index.js"',
+        // Did you mean to import "../es-modules/folder%2525with%20percentage%23/index.js"?
+        expected: / "\.\.\/folder%2525with%20percentage%23\/index\.js"\?/,
+        cwd: fixtureSubDir('es-modules/tla/'),
       },
       {
         input: 'import obj from "some_module/obj"',
-        expected: / some_module\/obj\.js\?/,
+        expected: / "some_module\/obj\.js"\?/,
+      },
+      {
+        input: 'import obj from "some_module/folder%25with percentage#/index.js"',
+        expected: / "some_module\/folder%2525with%20percentage%23\/index\.js"\?/,
+      },
+      {
+        input: 'import "@nodejsscope/pkg/index"',
+        expected: / "@nodejsscope\/pkg\/index\.js"\?/,
+      },
+      {
+        input: 'import obj from "lone_file.js"',
+        expected: /node_modules\/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);

test/es-module/test-esm-type-flag-cli-entry.mjs

@@ -26,7 +26,7 @@ describe('--experimental-default-type=module should not support extension search
       cwd: fixtures.path('es-modules/package-without-type'),
     });
 
-    match(stderr, /ENOENT.*Did you mean to import .*index\.js\?/s);
+    match(stderr, /ENOENT.*Did you mean to import .*index\.js"\?/s);
     strictEqual(stdout, '');
     strictEqual(code, 1);
     strictEqual(signal, null);

test/fixtures/es-modules/folder%25with percentage#/index.js

@@ -0,0 +1 @@
+'use strict';