diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 6cf968553bae6d..7efd7ef5e70286 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -1,10 +1,13 @@ 'use strict'; const { + ArrayPrototypeFilter, + ArrayPrototypeFind, ArrayPrototypeJoin, ArrayPrototypeMap, ArrayPrototypePush, FunctionPrototype, + Number, ObjectSetPrototypeOf, PromiseAll, PromiseResolve, @@ -14,13 +17,13 @@ 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(); @@ -28,6 +31,68 @@ 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 = 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 ? + `\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 { @@ -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'); diff --git a/test/es-module/test-esm-cjs-named-error.mjs b/test/es-module/test-esm-cjs-named-error.mjs index 4ef75a22f92674..f35603ec8d8804 100644 --- a/test/es-module/test-esm-cjs-named-error.mjs +++ b/test/es-module/test-esm-cjs-named-error.mjs @@ -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 }'); @@ -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`); diff --git a/test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs b/test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs new file mode 100644 index 00000000000000..5401f4fdef68bc --- /dev/null +++ b/test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs @@ -0,0 +1,4 @@ +import abc, { + comeOn as comeOnRenamed, + everybody, +} from './fail.cjs'; diff --git a/test/fixtures/es-modules/package-cjs-named-error/fail.cjs b/test/fixtures/es-modules/package-cjs-named-error/fail.cjs index cab82d3eb60d60..95bf062a357e21 100644 --- a/test/fixtures/es-modules/package-cjs-named-error/fail.cjs +++ b/test/fixtures/es-modules/package-cjs-named-error/fail.cjs @@ -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, }; diff --git a/test/fixtures/es-modules/package-cjs-named-error/long-multi-line.mjs b/test/fixtures/es-modules/package-cjs-named-error/long-multi-line.mjs new file mode 100644 index 00000000000000..cf4c017d58d390 --- /dev/null +++ b/test/fixtures/es-modules/package-cjs-named-error/long-multi-line.mjs @@ -0,0 +1,14 @@ +import { + comeOn, + // everybody, + one, + two, + three, + four, + five, + six, + seven, + eight, + nine, + ten, +} from "./fail.cjs";