esm: fix CJS named export error message for sync module jobs

Author: GeoffreyBooth

lib/internal/modules/esm/module_job.js

@@ -126,6 +126,58 @@ const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
   }
 };
 
+/**
+ * If `error` is a SyntaxError from V8 for a missing named export on a CJS module, rewrite its message to the friendlier
+ * "Named export '...' not found..." form. Must be called after `decorateErrorStack(error)` so that the arrow (source
+ * context with the import statement text) has been prepended to `error.stack`.
+ * @param {Error} error
+ * @param {ModuleWrap} module The parent module that triggered the instantiation.
+ * @param {boolean[]} commonJsDeps Per-request array indicating whether each dependency is a CJS module, aligned with
+ *   `module.getModuleRequests()`.
+ */
+const handleCJSNamedExportError = (error, module, commonJsDeps) => {
+  // TODO(@bcoe): Add source map support to exception that occurs as result
+  // of missing named export. This is currently not possible because
+  // stack trace originates in module_job, not the file itself. A hidden
+  // symbol with filename could be set in node_errors.cc to facilitate this.
+  if (!getSourceMapsSupport().enabled &&
+      StringPrototypeIncludes(error.message,
+                              ' does not provide an export named')) {
+    const splitStack = StringPrototypeSplit(error.stack, '\n', 2);
+    const { 1: childSpecifier, 2: name } = RegExpPrototypeExec(
+      /module '(.*)' does not provide an export named '(.+)'/,
+      error.message);
+    const moduleRequests = module.getModuleRequests();
+    let isCommonJS = false;
+    for (let i = 0; i < moduleRequests.length; ++i) {
+      if (moduleRequests[i].specifier === childSpecifier) {
+        isCommonJS = commonJsDeps[i];
+        break;
+      }
+    }
+    if (isCommonJS) {
+      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 destructuring assignment by
+      // just parsing the error stack.
+      const oneLineNamedImports = RegExpPrototypeExec(/{.*}/, importStatement);
+      const destructuringAssignment = oneLineNamedImports &&
+        RegExpPrototypeSymbolReplace(/\s+as\s+/g, oneLineNamedImports, ': ');
+      error.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` : ''}`;
+      const newStack = StringPrototypeSplit(error.stack, '\n');
+      newStack[3] = `SyntaxError: ${error.message}`;
+      error.stack = ArrayPrototypeJoin(newStack, '\n');
+    }
+  }
+};
+
 class ModuleJobBase {
   constructor(loader, url, importAttributes, phase, isMain, inspectBrk) {
     assert(typeof phase === 'number');
@@ -325,50 +377,10 @@ class ModuleJob extends ModuleJobBase {
       } else {
         this.module.instantiate();
       }
-    } catch (e) {
-      decorateErrorStack(e);
-      // TODO(@bcoe): Add source map support to exception that occurs as result
-      // of missing named export. This is currently not possible because
-      // stack trace originates in module_job, not the file itself. A hidden
-      // symbol with filename could be set in node_errors.cc to facilitate this.
-      if (!getSourceMapsSupport().enabled &&
-          StringPrototypeIncludes(e.message,
-                                  ' does not provide an export named')) {
-        const splitStack = StringPrototypeSplit(e.stack, '\n', 2);
-        const { 1: childSpecifier, 2: name } = RegExpPrototypeExec(
-          /module '(.*)' does not provide an export named '(.+)'/,
-          e.message);
-        const moduleRequests = this.module.getModuleRequests();
-        let isCommonJS = false;
-        for (let i = 0; i < moduleRequests.length; ++i) {
-          if (moduleRequests[i].specifier === childSpecifier) {
-            isCommonJS = this.commonJsDeps[i];
-            break;
-          }
-        }
-
-        if (isCommonJS) {
-          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 destructuring assignment by
-          // just parsing the error stack.
-          const oneLineNamedImports = RegExpPrototypeExec(/{.*}/, importStatement);
-          const destructuringAssignment = oneLineNamedImports &&
-            RegExpPrototypeSymbolReplace(/\s+as\s+/g, oneLineNamedImports, ': ');
-          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` : ''}`;
-          const newStack = StringPrototypeSplit(e.stack, '\n');
-          newStack[3] = `SyntaxError: ${e.message}`;
-          e.stack = ArrayPrototypeJoin(newStack, '\n');
-        }
-      }
-      throw e;
+    } catch (error) {
+      decorateErrorStack(error);
+      handleCJSNamedExportError(error, this.module, this.commonJsDeps);
+      throw error;
     }
 
     for (const dependencyJob of jobsInGraph) {
@@ -510,7 +522,13 @@ class ModuleJobSync extends ModuleJobBase {
     }
     if (status < kInstantiated) {
       // Fresh module: instantiate it now (links were already resolved synchronously in constructor)
-      this.module.instantiate();
+      try {
+        this.module.instantiate();
+      } catch (error) {
+        decorateErrorStack(error);
+        handleCJSNamedExportError(error, this.module, this.commonJsDeps);
+        throw error;
+      }
     }
     // `status === kInstantiated`: either just instantiated above, or previously instantiated
     // but evaluation was deferred (e.g. TLA detected by a prior `runSync()` call)