esm: fix hint on invalid module specifier

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 =
@@ -912,6 +912,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 {
@@ -924,29 +925,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;
@@ -1154,14 +1164,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';