From 1726a569e232e363558ad7da66bfce449defd78b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 19 Dec 2023 19:11:01 +0100 Subject: [PATCH 1/7] esm: fix hint on invalid module specifier --- lib/internal/modules/esm/resolve.js | 47 ++++++++++--------- ...est-esm-module-not-found-commonjs-hint.mjs | 31 +++++++++--- .../folder%25with percentage?/index.js | 1 + test/fixtures/node_modules/lone_file.js | 1 + .../folder%25with percentage?/index.js | 1 + 5 files changed, 53 insertions(+), 28 deletions(-) create mode 100644 test/fixtures/es-modules/folder%25with percentage?/index.js create mode 100644 test/fixtures/node_modules/lone_file.js create mode 100644 test/fixtures/node_modules/some_module/folder%25with percentage?/index.js diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index c40f81200de5fc..e7fafaee35df7b 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 = @@ -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(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 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 index 51633564f81458..ac58012e751a20 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,26 +7,43 @@ 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 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/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'; From 7324ce9edb6cd4a76592e4f7a0284184552b5343 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 19 Dec 2023 22:21:46 +0100 Subject: [PATCH 2/7] fixup! esm: fix hint on invalid module specifier --- lib/internal/modules/esm/resolve.js | 4 +++- ...test-esm-module-not-found-commonjs-hint.mjs | 18 +++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index e7fafaee35df7b..eb081a0d7d70b6 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -950,6 +950,8 @@ function resolveAsCommonJS(specifier, parentURL) { ), '/', ); + } else { + found = pathToFileURL(found); } } return found; @@ -1162,7 +1164,7 @@ function decorateErrorWithCommonJSHints(error, specifier, parentURL) { if (found && found !== specifier) { // Modify the stack and message string to include the hint const endOfFirstLine = StringPrototypeIndexOf(error.stack, '\n'); - const hint = `Did you mean to import ${found}?`; + const hint = `Did you mean to import ${JSONStringify(found)}?`; error.stack = StringPrototypeSlice(error.stack, 0, endOfFirstLine) + '\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 ac58012e751a20..e2ed242de59016 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 @@ -11,31 +11,31 @@ describe('ESM: module not found hint', { concurrency: true }, () => { 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\?/, + // Did you mean to import "./es-modules/folder%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\?/, + // Did you mean to import "../es-modules/folder%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\?/, + 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\?/, + 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).*$/, + expected: /node_modules\/lone_file\.js"\?/, }, ] ) it('should cite a variant form', async () => { From 88e72c54464928a132b305c01920036ae7e67a8e Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 19 Dec 2023 22:30:27 +0100 Subject: [PATCH 3/7] fixup! esm: fix hint on invalid module specifier --- lib/internal/modules/esm/resolve.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index eb081a0d7d70b6..34c7fce50442c0 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -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 { @@ -951,7 +952,7 @@ function resolveAsCommonJS(specifier, parentURL) { '/', ); } else { - found = pathToFileURL(found); + found = `${pathToFileURL(found)}`; } } return found; From 226a9456d3185c3e0811486c7d800f1c717c4c85 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 20 Dec 2023 19:25:15 +0100 Subject: [PATCH 4/7] fixup! fixup! esm: fix hint on invalid module specifier --- lib/internal/modules/esm/resolve.js | 5 +++-- test/es-module/test-esm-type-flag-cli-entry.mjs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 34c7fce50442c0..035e9f7ac74e5f 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -940,7 +940,8 @@ function resolveAsCommonJS(specifier, parentURL) { } else if (isBareSpecifier(specifier)) { // If it is a bare specifier return the relative path within the // module - const pkg = StringPrototypeSlice(specifier, 0, StringPrototypeIndexOf(specifier, '/')); + const i = StringPrototypeIndexOf(specifier, '/'); + const pkg = i === -1 ? specifier : StringPrototypeSlice(specifier, 0, i); const index = StringPrototypeIndexOf(found, `node_modules${sep}${pkg}${sep}`); if (index !== -1) { found = ArrayPrototypeJoin( @@ -1162,7 +1163,7 @@ function defaultResolve(specifier, context = {}) { */ function decorateErrorWithCommonJSHints(error, specifier, parentURL) { const found = resolveAsCommonJS(specifier, parentURL); - if (found && found !== specifier) { + if (found && found !== specifier) { // Don't suggest the same input the user provided. // Modify the stack and message string to include the hint const endOfFirstLine = StringPrototypeIndexOf(error.stack, '\n'); const hint = `Did you mean to import ${JSONStringify(found)}?`; 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); From 67439d1b8802dfdfd0751629c9be66c19b87803b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 21 Dec 2023 11:52:23 +0100 Subject: [PATCH 5/7] fixup! fixup! fixup! esm: fix hint on invalid module specifier --- lib/internal/modules/esm/resolve.js | 7 ++++--- test/es-module/test-esm-module-not-found-commonjs-hint.mjs | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 035e9f7ac74e5f..d520cc5f0480a9 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -942,11 +942,12 @@ function resolveAsCommonJS(specifier, parentURL) { // module const i = StringPrototypeIndexOf(specifier, '/'); const pkg = i === -1 ? specifier : StringPrototypeSlice(specifier, 0, i); - const index = StringPrototypeIndexOf(found, `node_modules${sep}${pkg}${sep}`); + const needle = `node_modules${sep}${pkg}${sep}`; + const index = StringPrototypeIndexOf(found, needle); if (index !== -1) { - found = ArrayPrototypeJoin( + found = pkg + '/' + ArrayPrototypeJoin( ArrayPrototypeMap( - StringPrototypeSplit(StringPrototypeSlice(found, index + `node_modules${sep}`.length), sep), + StringPrototypeSplit(StringPrototypeSlice(found, index + needle.length), sep), // Escape URL-special characters to avoid generating a incorrect suggestion encodeURIComponent, ), 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 e2ed242de59016..38a4cd290e21d2 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 @@ -33,6 +33,10 @@ describe('ESM: module not found hint', { concurrency: true }, () => { input: 'import obj from "some_module/folder%25with percentage?/index.js"', expected: / "some_module\/folder%2525with%20percentage%3F\/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"\?/, From 0523d28e26f75b8fdd9b210fd6147e922ac1e689 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 21 Dec 2023 12:03:26 +0100 Subject: [PATCH 6/7] fixup! fixup! fixup! esm: fix hint on invalid module specifier fix for Windows (`?` is illegal in file names apparently) --- .../test-esm-module-not-found-commonjs-hint.mjs | 16 ++++++++-------- .../index.js | 0 .../pkg}/index.js | 0 .../folder%25with percentage#/index.js | 1 + 4 files changed, 9 insertions(+), 8 deletions(-) rename test/fixtures/es-modules/{folder%25with percentage? => folder%25with percentage#}/index.js (100%) rename test/fixtures/node_modules/{some_module/folder%25with percentage? => @nodejsscope/pkg}/index.js (100%) create mode 100644 test/fixtures/node_modules/some_module/folder%25with percentage#/index.js 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 38a4cd290e21d2..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 @@ -15,14 +15,14 @@ describe('ESM: module not found hint', { concurrency: true }, () => { expected: / "\.\/print-error-message\.js"\?/, }, { - input: 'import "./es-modules/folder%25with percentage?/index.js"', - // Did you mean to import "./es-modules/folder%2525with%20percentage%3F/index.js"? - expected: / "\.\/es-modules\/folder%2525with%20percentage%3F\/index\.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%3F/index.js"? - expected: / "\.\.\/folder%2525with%20percentage%3F\/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/'), }, { @@ -30,8 +30,8 @@ describe('ESM: module not found hint', { concurrency: true }, () => { 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 "some_module/folder%25with percentage#/index.js"', + expected: / "some_module\/folder%2525with%20percentage%23\/index\.js"\?/, }, { input: 'import "@nodejsscope/pkg/index"', diff --git a/test/fixtures/es-modules/folder%25with percentage?/index.js b/test/fixtures/es-modules/folder%25with percentage#/index.js similarity index 100% rename from test/fixtures/es-modules/folder%25with percentage?/index.js rename to test/fixtures/es-modules/folder%25with percentage#/index.js diff --git a/test/fixtures/node_modules/some_module/folder%25with percentage?/index.js b/test/fixtures/node_modules/@nodejsscope/pkg/index.js similarity index 100% rename from test/fixtures/node_modules/some_module/folder%25with percentage?/index.js rename to test/fixtures/node_modules/@nodejsscope/pkg/index.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 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'; From 38f4580dbff46ce2919b16add54afb1798445fa5 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 23 Dec 2023 12:56:45 +0100 Subject: [PATCH 7/7] fixup! fixup! fixup! esm: fix hint on invalid module specifier --- lib/internal/modules/esm/resolve.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index d520cc5f0480a9..54cdbe8e6175ad 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -942,8 +942,8 @@ function resolveAsCommonJS(specifier, parentURL) { // module const i = StringPrototypeIndexOf(specifier, '/'); const pkg = i === -1 ? specifier : StringPrototypeSlice(specifier, 0, i); - const needle = `node_modules${sep}${pkg}${sep}`; - const index = StringPrototypeIndexOf(found, needle); + const needle = `${sep}node_modules${sep}${pkg}${sep}`; + const index = StringPrototypeLastIndexOf(found, needle); if (index !== -1) { found = pkg + '/' + ArrayPrototypeJoin( ArrayPrototypeMap(