Skip to content

Commit

Permalink
esm: fix hint on invalid module specifier
Browse files Browse the repository at this point in the history
PR-URL: #51223
Fixes: #51216
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
aduh95 authored and RafaelGSS committed Jan 2, 2024
1 parent 8412751 commit 47548d9
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 31 deletions.
54 changes: 32 additions & 22 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
const {
ArrayIsArray,
ArrayPrototypeJoin,
ArrayPrototypeShift,
ArrayPrototypeMap,
JSONStringify,
ObjectGetOwnPropertyNames,
ObjectPrototypeHasOwnProperty,
RegExp,
RegExpPrototypeExec,
RegExpPrototypeSymbolReplace,
SafeMap,
Expand All @@ -21,6 +20,7 @@ const {
StringPrototypeSlice,
StringPrototypeSplit,
StringPrototypeStartsWith,
encodeURIComponent,
} = primordials;
const internalFS = require('internal/fs/utils');
const { BuiltinModule } = require('internal/bootstrap/realm');
Expand All @@ -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 =
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
Expand Down Expand Up @@ -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}`;
}
}
Expand Down
37 changes: 29 additions & 8 deletions test/es-module/test-esm-module-not-found-commonjs-hint.mjs
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-type-flag-cli-entry.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
'use strict';
1 change: 1 addition & 0 deletions test/fixtures/node_modules/@nodejsscope/pkg/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/node_modules/lone_file.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 47548d9

Please sign in to comment.