Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: improve named cjs import errors #35453

Closed
wants to merge 11 commits into from
87 changes: 75 additions & 12 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
'use strict';

const {
ArrayPrototypeFilter,
ArrayPrototypeFind,
ArrayPrototypeJoin,
ctavan marked this conversation as resolved.
Show resolved Hide resolved
ArrayPrototypeMap,
ArrayPrototypePush,
FunctionPrototype,
Number,
ObjectSetPrototypeOf,
PromiseAll,
PromiseResolve,
Expand All @@ -14,20 +17,82 @@ const {
SafeSet,
StringPrototypeIncludes,
StringPrototypeMatch,
StringPrototypeReplace,
StringPrototypeSplit,
} = primordials;

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

const { decorateErrorStack } = require('internal/util');
const { fileURLToPath } = require('internal/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');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to "lazy-load" inline here? My assumption is that this code is executed exactly once before the node process crashes. Is my assumption correct?

I saw a different way of lazy loading acorn in assert

node/lib/assert.js

Lines 216 to 218 in 4a6005c

// Lazy load acorn.
if (parseExpressionAt === undefined) {
const acorn = require('internal/deps/acorn/acorn/dist/acorn');
.


const code = readFileSync(file, { encoding: 'utf8' });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to readFileSync?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code would run only if the process is crashing, right? I think it's perfectly fine to use readFileSync for that kind of things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is precisely the question: I don't understand 100% yet of this error will always lead to termination of the node process. If so, I also think that readFileSync is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I find a way to parse the input file in a streaming fashion… Since imports typically come in the beginning of a file this would allow us to reduce the parsing work to a minimum… But maybe that's all unnecessary micro-optimizations for the crash case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been benchmarked already, see nodejs/cjs-module-lexer#4 (comment). The gist of it is that streaming is not worth it for most JS files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context!

Great, so the one possible optimization would be, having read the entire file, around not parsing the entire file with with acorn, see #35453 (comment)

const parsed = parse(code, {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way around parsing the full file? I tried working with parseExpressionAt to only part the first few lines of a file but didn't have success (received unexpected token errors resulting from the import statements).

sourceType: 'module',
locations: true,
});
let start = 0;
let node;
do {
node = findNodeAt(parsed, start);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if findNodeAt returns undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you imagine a test case that would reproduce this?

Copy link
Contributor

@aduh95 aduh95 Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I assume it may happen because it's in your while condition.

Have you tried to use findNodeAround instead of findNodeAt? It seems to fit better our use case.

Suggested change
node = findNodeAt(parsed, start);
node = findNodeAround(parsed, lineNumber, 'ImportDeclaration');

start = node.node.end + 1;

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

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

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

const joinString = ', ';
let totalLength = 0;
const imports = ArrayPrototypeMap(
ArrayPrototypeFilter(
node.node.specifiers,
(specifier) => (specifier.type === 'ImportSpecifier'),
),
(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 ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My line splitting logic is pretty "simplistic"… Any idea on how to do this better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we do anything similar elsewhere? a nice to have would be checking the terminal width, rather than assuming 80.

`\n${ArrayPrototypeJoin(
ArrayPrototypeMap(imports, (i) => ` ${i}`),
',\n',
)}\n` :
` ${ArrayPrototypeJoin(imports, joinString)} `;

return `\n\nimport ${defaultImport} from '${node.node.source.value}';\n` +
`const {${destructuringAssignment}} = ${defaultImport};\n`;
} while (node === undefined || node.node.loc.start.line <= lineNumber);
assert.fail('Could not find erroneous import statement');
}

/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of
* its dependencies, over time. */
class ModuleJob {
Expand Down Expand Up @@ -117,21 +182,19 @@ class ModuleJob {
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, ': ');
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
46 changes: 40 additions & 6 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,41 @@ 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 expectedLongMultiLine = errTemplate('./fail.cjs', 'one',
'{\n' +
' comeOn,\n' +
' one,\n' +
' two,\n' +
' three,\n' +
' four,\n' +
' five,\n' +
' six,\n' +
' seven,\n' +
' eight,\n' +
' nine,\n' +
' ten\n' +
'}');

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,12 +64,26 @@ 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,
}, 'should correctly format named imports across multiple lines');
message: expectedMultiLine,
}, 'should correctly format named multi-line imports');

rejects(async () => {
await import(`${fixtureBase}/long-multi-line.mjs`);
}, {
name: 'SyntaxError',
message: expectedLongMultiLine,
}, 'should correctly format named very long multi-line imports');

rejects(async () => {
await import(`${fixtureBase}/json-hack.mjs`);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import abc, {
comeOn as comeOnRenamed,
everybody,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reasons I don't understand this import statement only errors out on line 3 (everybody). I do not understand why it doesn't error out on the renamed import statement. If I swap the order it will error on line 2 (still everybody).

Any clues why this is the case?

} from './fail.cjs';
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 10 additions & 0 deletions test/fixtures/es-modules/package-cjs-named-error/fail.cjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@
module.exports = {
comeOn: 'fhqwhgads',
everybody: 'to the limit',
one: 1,
two: 2,
three: 3,
four: 4,
five: 5,
six: 6,
seven: 7,
eight: 8,
nine: 9,
ten: 10,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {
comeOn,
// everybody,
one,
two,
three,
four,
five,
six,
seven,
eight,
nine,
ten,
} from "./fail.cjs";