FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: fix hint on invalid module specifier · nodejs/node@38f4000 · GitHub

/ node Public

Commit 38f4000

Browse files
aduh95
authored and
richardlau
committed
esm: fix hint on invalid module specifier
PR-URL: #51223 Fixes: #51216 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 01a4104 commit 38f4000

7 files changed

Lines changed: 66 additions & 31 deletions

File tree

‎lib/internal/modules/esm/resolve.js‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,10 @@
33
const {
44
ArrayIsArray,
55
ArrayPrototypeJoin,
6-
ArrayPrototypeShift,
6+
ArrayPrototypeMap,
77
JSONStringify,
88
ObjectGetOwnPropertyNames,
99
ObjectPrototypeHasOwnProperty,
10-
RegExp,
1110
RegExpPrototypeExec,
1211
RegExpPrototypeSymbolReplace,
1312
SafeMap,
@@ -21,6 +20,7 @@ const {
2120
StringPrototypeSlice,
2221
StringPrototypeSplit,
2322
StringPrototypeStartsWith,
23+
encodeURIComponent,
2424
} = primordials;
2525
const internalFS = require('internal/fs/utils');
2626
const { BuiltinModule } = require('internal/bootstrap/realm');
@@ -30,7 +30,7 @@ const { getOptionValue } = require('internal/options');
3030
const policy = getOptionValue('--experimental-policy') ?
3131
require('internal/process/policy') :
3232
null;
33-
const { sep, relative, toNamespacedPath, resolve } = require('path');
33+
const { sep, posix: { relative: relativePosixPath }, toNamespacedPath, resolve } = require('path');
3434
const preserveSymlinks = getOptionValue('--preserve-symlinks');
3535
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
3636
const experimentalNetworkImports =
@@ -921,6 +921,7 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) {
921921
* Try to resolve an import as a CommonJS module.
922922
* @param {string} specifier - The specifier to resolve.
923923
* @param {string} parentURL - The base URL.
924+
* @returns {string | Buffer | false}
924925
*/
925926
function resolveAsCommonJS(specifier, parentURL) {
926927
try {
@@ -933,29 +934,38 @@ function resolveAsCommonJS(specifier, parentURL) {
933934
// If it is a relative specifier return the relative path
934935
// to the parent
935936
if (isRelativeSpecifier(specifier)) {
936-
found = relative(parent, found);
937-
// Add '.separator if the path does not start with '..separator'
937+
const foundURL = pathToFileURL(found).pathname;
938+
found = relativePosixPath(
939+
StringPrototypeSlice(parentURL, 'file://'.length, StringPrototypeLastIndexOf(parentURL, '/')),
940+
foundURL);
941+
942+
// Add './' if the path does not start with '../'
938943
// This should be a safe assumption because when loading
939944
// esm modules there should be always a file specified so
940945
// there should not be a specifier like '..' or '.'
941-
if (!StringPrototypeStartsWith(found, `..${sep}`)) {
942-
found = `.${sep}${found}`;
946+
if (!StringPrototypeStartsWith(found, '../')) {
947+
found = `./${found}`;
943948
}
944949
} else if (isBareSpecifier(specifier)) {
945950
// If it is a bare specifier return the relative path within the
946951
// module
947-
const pkg = StringPrototypeSplit(specifier, '/')[0];
948-
const index = StringPrototypeIndexOf(found, pkg);
952+
const i = StringPrototypeIndexOf(specifier, '/');
953+
const pkg = i === -1 ? specifier : StringPrototypeSlice(specifier, 0, i);
954+
const needle = `${sep}node_modules${sep}${pkg}${sep}`;
955+
const index = StringPrototypeLastIndexOf(found, needle);
949956
if (index !== -1) {
950-
found = StringPrototypeSlice(found, index);
957+
found = pkg + '/' + ArrayPrototypeJoin(
958+
ArrayPrototypeMap(
959+
StringPrototypeSplit(StringPrototypeSlice(found, index + needle.length), sep),
960+
// Escape URL-special characters to avoid generating a incorrect suggestion
961+
encodeURIComponent,
962+
),
963+
'/',
964+
);
965+
} else {
966+
found = `${pathToFileURL(found)}`;
951967
}
952968
}
953-
// Normalize the path separator to give a valid suggestion
954-
// on Windows
955-
if (process.platform === 'win32') {
956-
found = RegExpPrototypeSymbolReplace(new RegExp(`\\${sep}`, 'g'),
957-
found, '/');
958-
}
959969
return found;
960970
} catch {
961971
return false;
@@ -1163,14 +1173,14 @@ function defaultResolve(specifier, context = {}) {
11631173
*/
11641174
function decorateErrorWithCommonJSHints(error, specifier, parentURL) {
11651175
const found = resolveAsCommonJS(specifier, parentURL);
1166-
if (found) {
1176+
if (found && found !== specifier) { // Don't suggest the same input the user provided.
11671177
// Modify the stack and message string to include the hint
1168-
const lines = StringPrototypeSplit(error.stack, '\n');
1169-
const hint = `Did you mean to import ${found}?`;
1178+
const endOfFirstLine = StringPrototypeIndexOf(error.stack, '\n');
1179+
const hint = `Did you mean to import ${JSONStringify(found)}?`;
11701180
error.stack =
1171-
ArrayPrototypeShift(lines) + '\n' +
1172-
hint + '\n' +
1173-
ArrayPrototypeJoin(lines, '\n');
1181+
StringPrototypeSlice(error.stack, 0, endOfFirstLine) + '\n' +
1182+
hint +
1183+
StringPrototypeSlice(error.stack, endOfFirstLine);
11741184
error.message += `\n${hint}`;
11751185
}
11761186
}

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

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,53 @@
11
import { spawnPromisified } from '../common/index.mjs';
2-
import { fixturesDir } from '../common/fixtures.mjs';
2+
import { fixturesDir, fileURL as fixtureSubDir } from '../common/fixtures.mjs';
33
import { match, notStrictEqual } from 'node:assert';
44
import { execPath } from 'node:process';
55
import { describe, it } from 'node:test';
66

77

88
describe('ESM: module not found hint', { concurrency: true }, () => {
99
for (
10-
const { input, expected }
10+
const { input, expected, cwd = fixturesDir }
1111
of [
1212
{
1313
input: 'import "./print-error-message"',
14-
// Did you mean to import ../print-error-message.js?
15-
expected: / \.\.\/print-error-message\.js\?/,
14+
// Did you mean to import "./print-error-message.js"?
15+
expected: / "\.\/print-error-message\.js"\?/,
16+
},
17+
{
18+
input: 'import "./es-modules/folder%25with percentage#/index.js"',
19+
// Did you mean to import "./es-modules/folder%2525with%20percentage%23/index.js"?
20+
expected: / "\.\/es-modules\/folder%2525with%20percentage%23\/index\.js"\?/,
21+
},
22+
{
23+
input: 'import "../folder%25with percentage#/index.js"',
24+
// Did you mean to import "../es-modules/folder%2525with%20percentage%23/index.js"?
25+
expected: / "\.\.\/folder%2525with%20percentage%23\/index\.js"\?/,
26+
cwd: fixtureSubDir('es-modules/tla/'),
1627
},
1728
{
1829
input: 'import obj from "some_module/obj"',
19-
expected: / some_module\/obj\.js\?/,
30+
expected: / "some_module\/obj\.js"\?/,
31+
},
32+
{
33+
input: 'import obj from "some_module/folder%25with percentage#/index.js"',
34+
expected: / "some_module\/folder%2525with%20percentage%23\/index\.js"\?/,
35+
},
36+
{
37+
input: 'import "@nodejsscope/pkg/index"',
38+
expected: / "@nodejsscope\/pkg\/index\.js"\?/,
39+
},
40+
{
41+
input: 'import obj from "lone_file.js"',
42+
expected: /node_modules\/lone_file\.js"\?/,
2043
},
2144
]
2245
) it('should cite a variant form', async () => {
2346
const { code, stderr } = await spawnPromisified(execPath, [
2447
'--input-type=module',
2548
'--eval',
2649
input,
27-
], {
28-
cwd: fixturesDir,
29-
});
50+
], { cwd });
3051

3152
match(stderr, expected);
3253
notStrictEqual(code, 0);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe('--experimental-default-type=module should not support extension search
2626
cwd: fixtures.path('es-modules/package-without-type'),
2727
});
2828

29-
match(stderr, /ENOENT.*Did you mean to import .*index\.js\?/s);
29+
match(stderr, /ENOENT.*Did you mean to import .*index\.js"\?/s);
3030
strictEqual(stdout, '');
3131
strictEqual(code, 1);
3232
strictEqual(signal, null);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
'use strict';

‎test/fixtures/node_modules/@nodejsscope/pkg/index.js‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/node_modules/lone_file.js‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/node_modules/some_module/folder%25with percentage#/index.js‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL