diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index c40f81200de5fc..54cdbe8e6175ad 100644 --- 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 = @@ -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}`; } } 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 index 51633564f81458..8c0187fe89066a 100644 --- 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,5 +1,5 @@ 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'; @@ -7,16 +7,39 @@ 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 () => { @@ -24,9 +47,7 @@ describe('ESM: module not found hint', { concurrency: true }, () => { '--input-type=module', '--eval', input, - ], { - cwd: fixturesDir, - }); + ], { cwd }); match(stderr, expected); notStrictEqual(code, 0); diff --git a/test/es-module/test-esm-type-flag-cli-entry.mjs b/test/es-module/test-esm-type-flag-cli-entry.mjs index 002840751532ff..b9315b1bb58992 100644 --- a/test/es-module/test-esm-type-flag-cli-entry.mjs +++ b/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); diff --git a/test/fixtures/es-modules/folder%25with percentage#/index.js b/test/fixtures/es-modules/folder%25with percentage#/index.js new file mode 100644 index 00000000000000..ad9a93a7c160f0 --- /dev/null +++ b/test/fixtures/es-modules/folder%25with percentage#/index.js @@ -0,0 +1 @@ +'use strict'; diff --git a/test/fixtures/node_modules/@nodejsscope/pkg/index.js b/test/fixtures/node_modules/@nodejsscope/pkg/index.js new file mode 100644 index 00000000000000..ad9a93a7c160f0 --- /dev/null +++ b/test/fixtures/node_modules/@nodejsscope/pkg/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 new file mode 100644 index 00000000000000..ad9a93a7c160f0 --- /dev/null +++ b/test/fixtures/node_modules/lone_file.js @@ -0,0 +1 @@ +'use strict'; 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 new file mode 100644 index 00000000000000..ad9a93a7c160f0 --- /dev/null +++ b/test/fixtures/node_modules/some_module/folder%25with percentage#/index.js @@ -0,0 +1 @@ +'use strict';