Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
esm: improve commonjs hint on module not found
Run CommonJS resolver only if `error.code` is ERR_MODULE_NOT_FOUND.
Avoid using absolute paths in hint by:
* using a parent-relative path if the specifier is a relative path
* using a `pkg/x.js` format if the specifier is bare (e.g. `pkg/x`)

PR-URL: #31906
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
  • Loading branch information
dnlup authored and targos committed May 13, 2020
1 parent 6b02359 commit daf1d84
Show file tree
Hide file tree
Showing 9 changed files with 164 additions and 6 deletions.
90 changes: 84 additions & 6 deletions lib/internal/modules/esm/resolve.js
Expand Up @@ -2,17 +2,22 @@

const {
ArrayIsArray,
ArrayPrototypeJoin,
ArrayPrototypeShift,
JSONParse,
JSONStringify,
ObjectFreeze,
ObjectGetOwnPropertyNames,
ObjectPrototypeHasOwnProperty,
RegExp,
SafeMap,
SafeSet,
StringPrototypeEndsWith,
StringPrototypeIncludes,
StringPrototypeIndexOf,
StringPrototypeReplace,
StringPrototypeSlice,
StringPrototypeSplit,
StringPrototypeStartsWith,
StringPrototypeSubstr,
} = primordials;
Expand All @@ -29,8 +34,8 @@ const {
Stats,
} = require('fs');
const { getOptionValue } = require('internal/options');
const { sep } = require('path');

const { sep, relative } = require('path');
const { Module: CJSModule } = require('internal/modules/cjs/loader');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
const typeFlag = getOptionValue('--input-type');
Expand Down Expand Up @@ -611,9 +616,11 @@ function packageResolve(specifier, base, conditions) {
throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base));
}

function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) {
if (specifier === '') return false;
if (specifier[0] === '/') return true;
function isBareSpecifier(specifier) {
return specifier[0] && specifier[0] !== '/' && specifier[0] !== '.';
}

function isRelativeSpecifier(specifier) {
if (specifier[0] === '.') {
if (specifier.length === 1 || specifier[1] === '/') return true;
if (specifier[1] === '.') {
Expand All @@ -623,6 +630,12 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) {
return false;
}

function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) {
if (specifier === '') return false;
if (specifier[0] === '/') return true;
return isRelativeSpecifier(specifier);
}

/**
* @param {string} specifier
* @param {URL} base
Expand All @@ -645,6 +658,51 @@ function moduleResolve(specifier, base, conditions) {
return finalizeResolution(resolved, base);
}

/**
* Try to resolve an import as a CommonJS module
* @param {string} specifier
* @param {string} parentURL
* @returns {boolean|string}
*/
function resolveAsCommonJS(specifier, parentURL) {
try {
const parent = fileURLToPath(parentURL);
const tmpModule = new CJSModule(parent, null);
tmpModule.paths = CJSModule._nodeModulePaths(parent);

let found = CJSModule._resolveFilename(specifier, tmpModule, false);

// 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'
// 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}`;
}
} 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);
if (index !== -1) {
found = StringPrototypeSlice(found, index);
}
}
// Normalize the path separator to give a valid suggestion
// on Windows
if (process.platform === 'win32') {
found = StringPrototypeReplace(found, new RegExp(`\\${sep}`, 'g'), '/');
}
return found;
} catch {
return false;
}
}

function defaultResolve(specifier, context = {}, defaultResolveUnused) {
let { parentURL, conditions } = context;
let parsed;
Expand Down Expand Up @@ -685,7 +743,27 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) {
}

conditions = getConditionsSet(conditions);
let url = moduleResolve(specifier, parentURL, conditions);
let url;
try {
url = moduleResolve(specifier, parentURL, conditions);
} catch (error) {
// Try to give the user a hint of what would have been the
// resolved CommonJS module
if (error.code === 'ERR_MODULE_NOT_FOUND') {
const found = resolveAsCommonJS(specifier, parentURL);
if (found) {
// Modify the stack and message string to include the hint
const lines = StringPrototypeSplit(error.stack, '\n');
const hint = `Did you mean to import ${found}?`;
error.stack =
ArrayPrototypeShift(lines) + '\n' +
hint + '\n' +
ArrayPrototypeJoin(lines, '\n');
error.message += `\n${hint}`;
}
}
throw error;
}

if (isMain ? !preserveSymlinksMain : !preserveSymlinks) {
const urlPath = fileURLToPath(url);
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs
@@ -0,0 +1,5 @@
'use strict';

import obj from 'some_module/obj';

throw new Error('Should have errored');
3 changes: 3 additions & 0 deletions test/fixtures/node_modules/some_module/index.js

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

3 changes: 3 additions & 0 deletions test/fixtures/node_modules/some_module/obj.js

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

12 changes: 12 additions & 0 deletions test/fixtures/node_modules/some_module/package.json

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

17 changes: 17 additions & 0 deletions test/message/esm_loader_not_found_cjs_hint_bare.js
@@ -0,0 +1,17 @@
'use strict';

require('../common');
const { spawn } = require('child_process');
const { join } = require('path');
const { fixturesDir } = require('../common/fixtures');

spawn(
process.execPath,
[
join(fixturesDir, 'esm_loader_not_found_cjs_hint_bare.mjs')
],
{
cwd: fixturesDir,
stdio: 'inherit'
}
);
17 changes: 17 additions & 0 deletions test/message/esm_loader_not_found_cjs_hint_bare.out
@@ -0,0 +1,17 @@
(node:*) ExperimentalWarning: The ESM module loader is experimental.
internal/modules/run_main.js:*
internalBinding('errors').triggerUncaughtException(
^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*fixtures*node_modules*some_module*obj' imported from *test*fixtures*esm_loader_not_found_cjs_hint_bare.mjs
Did you mean to import some_module/obj.js?
at finalizeResolution (internal/modules/esm/resolve.js:*:*)
at packageResolve (internal/modules/esm/resolve.js:*:*)
at moduleResolve (internal/modules/esm/resolve.js:*:*)
at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*)
at Loader.resolve (internal/modules/esm/loader.js:*:*)
at Loader.getModuleJob (internal/modules/esm/loader.js:*:*)
at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:*:*)
at link (internal/modules/esm/module_job.js:*:*) {
code: 'ERR_MODULE_NOT_FOUND'
}
3 changes: 3 additions & 0 deletions test/message/esm_loader_not_found_cjs_hint_relative.mjs
@@ -0,0 +1,3 @@
// Flags: --experimental-loader ./test/common/fixtures
import '../common/index.mjs';
console.log('This should not be printed');
20 changes: 20 additions & 0 deletions test/message/esm_loader_not_found_cjs_hint_relative.out
@@ -0,0 +1,20 @@
(node:*) ExperimentalWarning: The ESM module loader is experimental.
(node:*) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
internal/modules/run_main.js:*
internalBinding('errors').triggerUncaughtException(
^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*common*fixtures' imported from *
Did you mean to import ./test/common/fixtures.js?
at finalizeResolution (internal/modules/esm/resolve.js:*:*)
at moduleResolve (internal/modules/esm/resolve.js:*:*)
at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*)
at Loader.resolve (internal/modules/esm/loader.js:*:*)
at Loader.getModuleJob (internal/modules/esm/loader.js:*:*)
at Loader.import (internal/modules/esm/loader.js:*:*)
at internal/process/esm_loader.js:*:*
at Object.initializeLoader (internal/process/esm_loader.js:*:*)
at runMainESM (internal/modules/run_main.js:*:*)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*) {
code: 'ERR_MODULE_NOT_FOUND'
}

0 comments on commit daf1d84

Please sign in to comment.