Implement synchronous instantiation for ModuleJobSync; enhance error handling

Author: GeoffreyBooth

lib/internal/modules/esm/loader.js

@@ -372,7 +372,7 @@ class ModuleLoader {
     return onImport.traceSync(() => {
       const request = { specifier: url, phase: kEvaluationPhase, attributes: kEmptyObject, __proto__: null };
       const job = this.getOrCreateModuleJob(undefined, request, kImportInImportedESM);
-      job.module.instantiate();
+      job.instantiateSync();
       if (job.module.hasAsyncGraph) {
         // Hand the job back so the caller can evaluate it through the ordinary
         // asynchronous entry point machinery, without re-resolving and re-linking

lib/internal/modules/esm/module_job.js

@@ -630,6 +630,16 @@ class ModuleJobSync extends ModuleJobBase {
     return PromiseResolve(this.module);
   }
 
+  instantiateSync() {
+    try {
+      this.module.instantiate();
+    } catch (error) {
+      decorateErrorStack(error);
+      handleCJSNamedExportError(error, this.module, this.commonJsDeps);
+      throw error;
+    }
+  }
+
   async run(isEntryPoint = false) {
     assert(this.shouldRunModule(this.phase));
     const status = this.module.getStatus();
@@ -650,13 +660,7 @@ class ModuleJobSync extends ModuleJobBase {
       // or one that was initially require()'d and is now being imported after its instantiation
       // failed (e.g. a missing named export). Try finishing the instantiation: if it succeeds,
       // proceed to evaluation, otherwise re-throw the instantiation error.
-      try {
-        this.module.instantiate();
-      } catch (error) {
-        decorateErrorStack(error);
-        handleCJSNamedExportError(error, this.module, this.commonJsDeps);
-        throw error;
-      }
+      this.instantiateSync();
     }
     // `status === kInstantiated`: either just instantiated above, or previously instantiated
     // but evaluation was deferred (e.g. TLA detected by a prior `runSync()` call)
@@ -703,8 +707,7 @@ class ModuleJobSync extends ModuleJobBase {
     assert(this.shouldRunModule(this.phase));
     // TODO(joyeecheung): Reject graphs with top-level await _before_ instantiation, so that the
     // async graph error supersedes instantiation (mismatch export) errors in the graph.
-    // TODO(joyeecheung): add the error decoration logic from the async instantiate.
-    this.module.instantiate();
+    this.instantiateSync();
     // On the deprecated async loader hook worker thread, dependencies linked by an
     // earlier import may not be walkable synchronously, so double-check with
     // V8 now that the graph is instantiated.

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

@@ -1,5 +1,7 @@
-import '../common/index.mjs';
+import { spawnPromisified } from '../common/index.mjs';
+import * as fixtures from '../common/fixtures.mjs';
 import assert from 'assert';
+import { execPath } from 'node:process';
 
 const fixtureBase = '../fixtures/es-modules/package-cjs-named-error';
 
@@ -75,3 +77,10 @@ await assert.rejects(async () => {
 await assert.rejects(async () => {
   await import(`${fixtureBase}/escaped-single-quote.mjs`);
 }, /import pkg from '\.\/oh'no\.cjs'/, 'should support relative specifiers with escaped single quote');
+
+const entryPoint = fixtures.path('es-modules', 'package-cjs-named-error', 'single-quote.mjs');
+const { code, stderr } = await spawnPromisified(execPath, [entryPoint]);
+assert.strictEqual(code, 1);
+assert.ok(stderr.includes(expectedRelative), 'entry point should show the CommonJS named export hint');
+assert.ok(stderr.includes("import { comeOn } from './fail.cjs';"),
+          'entry point error should include the source import statement');