Skip to content

Commit

Permalink
module: improve named cjs import errors
Browse files Browse the repository at this point in the history
When trying to import named exports from a CommonJS module an error is
thrown. Unfortunately the V8 error only contains the single line that
causes the error, it is therefore impossible to construct an equivalent
code consisting of default import + object descructuring assignment.

This was the reason why the example code was removed for multi line
import statements in nodejs#35275

To generate a helpful error messages for any case we can parse the file
where the error happens using acorn and construct a valid example code
from the parsed ImportDeclaration. This will work for _any_ valid import
statement.

Since this code is only executed shortly before the node process crashes
anyways performance should not be a concern here.

Fixes: nodejs#35259
Refs: nodejs#35275
  • Loading branch information
ctavan committed Jan 29, 2021
1 parent 0a993e1 commit 22f700c
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 25 deletions.
105 changes: 85 additions & 20 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {
ArrayPrototypeMap,
ArrayPrototypePush,
FunctionPrototype,
Number,
ObjectSetPrototypeOf,
PromiseAll,
PromiseResolve,
Expand All @@ -14,20 +15,78 @@ const {
SafeSet,
StringPrototypeIncludes,
StringPrototypeMatch,
StringPrototypeReplace,
StringPrototypeSplit,
} = primordials;

const { ModuleWrap } = internalBinding('module_wrap');

const { decorateErrorStack } = require('internal/util');
const { fileURLToPath } = require('url');
const assert = require('internal/assert');
const resolvedPromise = PromiseResolve();

const noop = FunctionPrototype;

let hasPausedEntry = false;

function extractExample(file, lineNumber) {
const { readFileSync } = require('fs');
const { parse } = require('internal/deps/acorn/acorn/dist/acorn');
const { findNodeAt } = require('internal/deps/acorn/acorn-walk/dist/walk');

const code = readFileSync(file, { encoding: 'utf8' });
const parsed = parse(code, {
sourceType: 'module',
locations: true,
});
let start = 0;
let node;
do {
node = findNodeAt(parsed, start);
start = node.node.end + 1;

if (node.node.loc.end.line < lineNumber) {
continue;
}

if (node.node.type !== 'ImportDeclaration') {
continue;
}

const defaultSpecifier = node.node.specifiers.find(
(specifier) => specifier.type === 'ImportDefaultSpecifier'
);
const defaultImport = defaultSpecifier
? defaultSpecifier.local.name
: 'pkg';

const joinString = ', ';
let totalLength = 0;
const imports = node.node.specifiers
.filter((specifier) => specifier.type === 'ImportSpecifier')
.map((specifier) => {
const statement =
specifier.local.name === specifier.imported.name
? `${specifier.imported.name}`
: `${specifier.imported.name}: ${specifier.local.name}`;
totalLength += statement.length + joinString.length;
return statement;
});

const boilerplate = `const { } = ${defaultImport};`;
const destructuringAssignment =
totalLength > 80 - boilerplate.length
? `\n${imports.map((i) => ` ${i}`).join(',\n')}\n`
: ` ${imports.join(joinString)} `;

return (
`\n\nimport ${defaultImport} from '${node.node.source.value}';\n` +
`const {${destructuringAssignment}} = ${defaultImport};\n`
);
} while (node === undefined || node.node.loc.start.line <= lineNumber);
return '';
}

/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of
* its dependencies, over time. */
class ModuleJob {
Expand Down Expand Up @@ -91,8 +150,11 @@ class ModuleJob {
}
jobsInGraph.add(moduleJob);
const dependencyJobs = await moduleJob.linked;
return PromiseAll(new SafeArrayIterator(
ArrayPrototypeMap(dependencyJobs, addJobsToDependencyGraph)));
return PromiseAll(
new SafeArrayIterator(
ArrayPrototypeMap(dependencyJobs, addJobsToDependencyGraph)
)
);
};
await addJobsToDependencyGraph(this);

Expand All @@ -106,32 +168,35 @@ class ModuleJob {
}
} catch (e) {
decorateErrorStack(e);
if (StringPrototypeIncludes(e.message,
' does not provide an export named')) {
if (
StringPrototypeIncludes(e.message, ' does not provide an export named')
) {
const splitStack = StringPrototypeSplit(e.stack, '\n');
const parentFileUrl = splitStack[0];
const { 1: childSpecifier, 2: name } = StringPrototypeMatch(
e.message,
/module '(.*)' does not provide an export named '(.+)'/);
const childFileURL =
await this.loader.resolve(childSpecifier, parentFileUrl);
/module '(.*)' does not provide an export named '(.+)'/
);
const childFileURL = await this.loader.resolve(
childSpecifier,
parentFileUrl
);
const format = await this.loader.getFormat(childFileURL);
if (format === 'commonjs') {
const importStatement = splitStack[1];
// TODO(@ctavan): The original error stack only provides the single
// line which causes the error. For multi-line import statements we
// cannot generate an equivalent object descructuring assignment by
// just parsing the error stack.
const oneLineNamedImports = StringPrototypeMatch(importStatement, /{.*}/);
const destructuringAssignment = oneLineNamedImports &&
StringPrototypeReplace(oneLineNamedImports, /\s+as\s+/g, ': ');
e.message = `Named export '${name}' not found. The requested module` +
const [, fileUrl, lineNumber] = StringPrototypeMatch(
parentFileUrl,
/^(.*):([0-9]+)$/
);
const example = extractExample(
fileURLToPath(fileUrl),
Number(lineNumber)
);
e.message =
`Named export '${name}' not found. The requested module` +
` '${childSpecifier}' is a CommonJS module, which may not support` +
' all module.exports as named exports.\nCommonJS modules can ' +
'always be imported via the default export, for example using:' +
`\n\nimport pkg from '${childSpecifier}';\n${
destructuringAssignment ?
`const ${destructuringAssignment} = pkg;\n` : ''}`;
example;
const newStack = StringPrototypeSplit(e.stack, '\n');
newStack[3] = `SyntaxError: ${e.message}`;
e.stack = ArrayPrototypeJoin(newStack, '\n');
Expand Down
22 changes: 17 additions & 5 deletions test/es-module/test-esm-cjs-named-error.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,26 @@ import { rejects } from 'assert';

const fixtureBase = '../fixtures/es-modules/package-cjs-named-error';

const errTemplate = (specifier, name, namedImports) =>
const errTemplate = (specifier, name, namedImports, defaultName) =>
`Named export '${name}' not found. The requested module` +
` '${specifier}' is a CommonJS module, which may not support ` +
'all module.exports as named exports.\nCommonJS modules can ' +
'always be imported via the default export, for example using:' +
`\n\nimport pkg from '${specifier}';\n` + (namedImports ?
`const ${namedImports} = pkg;\n` : '');
`\n\nimport ${defaultName || 'pkg'} from '${specifier}';\n` + (namedImports ?
`const ${namedImports} = ${defaultName || 'pkg'};\n` : '');

const expectedWithoutExample = errTemplate('./fail.cjs', 'comeOn');
const expectedMultiLine = errTemplate('./fail.cjs', 'comeOn',
'{ comeOn, everybody }');

const expectedRelative = errTemplate('./fail.cjs', 'comeOn', '{ comeOn }');

const expectedRenamed = errTemplate('./fail.cjs', 'comeOn',
'{ comeOn: comeOnRenamed }');

const expectedDefaultRenamed =
errTemplate('./fail.cjs', 'everybody', '{ comeOn: comeOnRenamed, everybody }',
'abc');

const expectedPackageHack =
errTemplate('./json-hack/fail.js', 'comeOn', '{ comeOn }');

Expand All @@ -44,11 +49,18 @@ rejects(async () => {
message: expectedRenamed
}, 'should correctly format named imports with renames');

rejects(async () => {
await import(`${fixtureBase}/default-and-renamed-import.mjs`);
}, {
name: 'SyntaxError',
message: expectedDefaultRenamed
}, 'should correctly format hybrid default and named imports with renames');

rejects(async () => {
await import(`${fixtureBase}/multi-line.mjs`);
}, {
name: 'SyntaxError',
message: expectedWithoutExample,
message: expectedMultiLine,
}, 'should correctly format named imports across multiple lines');

rejects(async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import abc, {
comeOn as comeOnRenamed,
everybody,
} from './fail.cjs';

0 comments on commit 22f700c

Please sign in to comment.