Skip to content

Commit 22f700c

Browse files
ctavanctavan
committed
module: improve named cjs import errors
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
1 parent 0a993e1 commit 22f700c

3 files changed

Lines changed: 106 additions & 25 deletions

File tree

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

Lines changed: 85 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const {
55
ArrayPrototypeMap,
66
ArrayPrototypePush,
77
FunctionPrototype,
8+
Number,
89
ObjectSetPrototypeOf,
910
PromiseAll,
1011
PromiseResolve,
@@ -14,20 +15,78 @@ const {
1415
SafeSet,
1516
StringPrototypeIncludes,
1617
StringPrototypeMatch,
17-
StringPrototypeReplace,
1818
StringPrototypeSplit,
1919
} = primordials;
2020

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

2323
const { decorateErrorStack } = require('internal/util');
24+
const { fileURLToPath } = require('url');
2425
const assert = require('internal/assert');
2526
const resolvedPromise = PromiseResolve();
2627

2728
const noop = FunctionPrototype;
2829

2930
let hasPausedEntry = false;
3031

32+
function extractExample(file, lineNumber) {
33+
const { readFileSync } = require('fs');
34+
const { parse } = require('internal/deps/acorn/acorn/dist/acorn');
35+
const { findNodeAt } = require('internal/deps/acorn/acorn-walk/dist/walk');
36+
37+
const code = readFileSync(file, { encoding: 'utf8' });
38+
const parsed = parse(code, {
39+
sourceType: 'module',
40+
locations: true,
41+
});
42+
let start = 0;
43+
let node;
44+
do {
45+
node = findNodeAt(parsed, start);
46+
start = node.node.end + 1;
47+
48+
if (node.node.loc.end.line < lineNumber) {
49+
continue;
50+
}
51+
52+
if (node.node.type !== 'ImportDeclaration') {
53+
continue;
54+
}
55+
56+
const defaultSpecifier = node.node.specifiers.find(
57+
(specifier) => specifier.type === 'ImportDefaultSpecifier'
58+
);
59+
const defaultImport = defaultSpecifier
60+
? defaultSpecifier.local.name
61+
: 'pkg';
62+
63+
const joinString = ', ';
64+
let totalLength = 0;
65+
const imports = node.node.specifiers
66+
.filter((specifier) => specifier.type === 'ImportSpecifier')
67+
.map((specifier) => {
68+
const statement =
69+
specifier.local.name === specifier.imported.name
70+
? `${specifier.imported.name}`
71+
: `${specifier.imported.name}: ${specifier.local.name}`;
72+
totalLength += statement.length + joinString.length;
73+
return statement;
74+
});
75+
76+
const boilerplate = `const { } = ${defaultImport};`;
77+
const destructuringAssignment =
78+
totalLength > 80 - boilerplate.length
79+
? `\n${imports.map((i) => ` ${i}`).join(',\n')}\n`
80+
: ` ${imports.join(joinString)} `;
81+
82+
return (
83+
`\n\nimport ${defaultImport} from '${node.node.source.value}';\n` +
84+
`const {${destructuringAssignment}} = ${defaultImport};\n`
85+
);
86+
} while (node === undefined || node.node.loc.start.line <= lineNumber);
87+
return '';
88+
}
89+
3190
/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of
3291
* its dependencies, over time. */
3392
class ModuleJob {
@@ -91,8 +150,11 @@ class ModuleJob {
91150
}
92151
jobsInGraph.add(moduleJob);
93152
const dependencyJobs = await moduleJob.linked;
94-
return PromiseAll(new SafeArrayIterator(
95-
ArrayPrototypeMap(dependencyJobs, addJobsToDependencyGraph)));
153+
return PromiseAll(
154+
new SafeArrayIterator(
155+
ArrayPrototypeMap(dependencyJobs, addJobsToDependencyGraph)
156+
)
157+
);
96158
};
97159
await addJobsToDependencyGraph(this);
98160

@@ -106,32 +168,35 @@ class ModuleJob {
106168
}
107169
} catch (e) {
108170
decorateErrorStack(e);
109-
if (StringPrototypeIncludes(e.message,
110-
' does not provide an export named')) {
171+
if (
172+
StringPrototypeIncludes(e.message, ' does not provide an export named')
173+
) {
111174
const splitStack = StringPrototypeSplit(e.stack, '\n');
112175
const parentFileUrl = splitStack[0];
113176
const { 1: childSpecifier, 2: name } = StringPrototypeMatch(
114177
e.message,
115-
/module '(.*)' does not provide an export named '(.+)'/);
116-
const childFileURL =
117-
await this.loader.resolve(childSpecifier, parentFileUrl);
178+
/module '(.*)' does not provide an export named '(.+)'/
179+
);
180+
const childFileURL = await this.loader.resolve(
181+
childSpecifier,
182+
parentFileUrl
183+
);
118184
const format = await this.loader.getFormat(childFileURL);
119185
if (format === 'commonjs') {
120-
const importStatement = splitStack[1];
121-
// TODO(@ctavan): The original error stack only provides the single
122-
// line which causes the error. For multi-line import statements we
123-
// cannot generate an equivalent object descructuring assignment by
124-
// just parsing the error stack.
125-
const oneLineNamedImports = StringPrototypeMatch(importStatement, /{.*}/);
126-
const destructuringAssignment = oneLineNamedImports &&
127-
StringPrototypeReplace(oneLineNamedImports, /\s+as\s+/g, ': ');
128-
e.message = `Named export '${name}' not found. The requested module` +
186+
const [, fileUrl, lineNumber] = StringPrototypeMatch(
187+
parentFileUrl,
188+
/^(.*):([0-9]+)$/
189+
);
190+
const example = extractExample(
191+
fileURLToPath(fileUrl),
192+
Number(lineNumber)
193+
);
194+
e.message =
195+
`Named export '${name}' not found. The requested module` +
129196
` '${childSpecifier}' is a CommonJS module, which may not support` +
130197
' all module.exports as named exports.\nCommonJS modules can ' +
131198
'always be imported via the default export, for example using:' +
132-
`\n\nimport pkg from '${childSpecifier}';\n${
133-
destructuringAssignment ?
134-
`const ${destructuringAssignment} = pkg;\n` : ''}`;
199+
example;
135200
const newStack = StringPrototypeSplit(e.stack, '\n');
136201
newStack[3] = `SyntaxError: ${e.message}`;
137202
e.stack = ArrayPrototypeJoin(newStack, '\n');

‎test/es-module/test-esm-cjs-named-error.mjs‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,26 @@ import { rejects } from 'assert';
33

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

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

14-
const expectedWithoutExample = errTemplate('./fail.cjs', 'comeOn');
14+
const expectedMultiLine = errTemplate('./fail.cjs', 'comeOn',
15+
'{ comeOn, everybody }');
1516

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

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

22+
const expectedDefaultRenamed =
23+
errTemplate('./fail.cjs', 'everybody', '{ comeOn: comeOnRenamed, everybody }',
24+
'abc');
25+
2126
const expectedPackageHack =
2227
errTemplate('./json-hack/fail.js', 'comeOn', '{ comeOn }');
2328

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

52+
rejects(async () => {
53+
await import(`${fixtureBase}/default-and-renamed-import.mjs`);
54+
}, {
55+
name: 'SyntaxError',
56+
message: expectedDefaultRenamed
57+
}, 'should correctly format hybrid default and named imports with renames');
58+
4759
rejects(async () => {
4860
await import(`${fixtureBase}/multi-line.mjs`);
4961
}, {
5062
name: 'SyntaxError',
51-
message: expectedWithoutExample,
63+
message: expectedMultiLine,
5264
}, 'should correctly format named imports across multiple lines');
5365

5466
rejects(async () => {

‎test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import abc, {
2+
comeOn as comeOnRenamed,
3+
everybody,
4+
} from './fail.cjs';

0 commit comments

Comments
 (0)