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 #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: #35259 Refs: #35275
nodejs · ctavan · Oct 1, 2020 · Oct 2, 2020 · Oct 2, 2020 · Oct 2, 2020
commit 22f700cbc72ebcdb58531ab754f61672a384348a
diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -5,6 +5,7 @@ const {
   ArrayPrototypeMap,
   ArrayPrototypePush,
   FunctionPrototype,
+  Number,
   ObjectSetPrototypeOf,
   PromiseAll,
   PromiseResolve,
@@ -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');
 // Lazy load acorn. 
 if (parseExpressionAt === undefined) { 
   const acorn = require('internal/deps/acorn/acorn/dist/acorn'); 
 // Lazy load acorn. 
 if (parseExpressionAt === undefined) { 
   const acorn = require('internal/deps/acorn/acorn/dist/acorn'); 
+
+  const code = readFileSync(file, { encoding: 'utf8' });
+  const parsed = parse(code, {
+    sourceType: 'module',
+    locations: true,
+  });
+  let start = 0;
+  let node;
+  do {
+    node = findNodeAt(parsed, start);
-    node = findNodeAt(parsed, start);
+    node = findNodeAround(parsed, lineNumber, 'ImportDeclaration');
-    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 = 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 {
@@ -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);
 
@@ -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');

diff --git a/test/es-module/test-esm-cjs-named-error.mjs b/test/es-module/test-esm-cjs-named-error.mjs
@@ -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 }');
 
@@ -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 () => {

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
@@ -0,0 +1,4 @@
+import abc, {
+  comeOn as comeOnRenamed,
+  everybody,
+} from './fail.cjs';