module: fix require(esm) on the module.register() hooks thread

On the async loader hooks thread, resolve kImportInRequiredESM and
kRequireInImportedCJS requests synchronously again, and always use
ModuleJobSync for kImportInRequiredESM. Otherwise require() of an ES
module with imports from a hooks module hit an internal assertion in
ModuleJobSync.syncLink().

Fixes: https://github.com/nodejs/node/issues/66767
Refs: https://github.com/nodejs/node/pull/62530
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: https://github.com/nodejs/node/pull/66770
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

lib/internal/modules/esm/loader.js

@@ -613,12 +613,13 @@ class ModuleLoader {
 
     const isMain = (parentURL === undefined);
     const inspectBrk = (isMain && getOptionValue('--inspect-brk'));
-    // Use ModuleJobSync whenever we're on the main thread (not the async loader hook worker),
+    // Use ModuleJobSync for kImportInRequiredESM, which must always be linked synchronously,
+    // and whenever we're on the main thread (not the async loader hook worker),
     // except for kRequireInImportedCJS (TODO: consolidate that case too).
     // TODO(joyeecheung): use ModuleJobSync for kRequireInImportedCJS too.
-    const ModuleJobCtor = (this.isForAsyncLoaderHookWorker ||
-                           requestType === kRequireInImportedCJS) ?
-      ModuleJob : ModuleJobSync;
+    const ModuleJobCtor = (requestType === kImportInRequiredESM ||
+                           (!this.isForAsyncLoaderHookWorker && requestType !== kRequireInImportedCJS)) ?
+      ModuleJobSync : ModuleJob;
     job = new ModuleJobCtor(
       this,
       url,
@@ -645,9 +646,12 @@ class ModuleLoader {
    */
   getOrCreateModuleJob(parentURL, request, requestType) {
     let maybePromise;
-    if (!this.isForAsyncLoaderHookWorker) {
+    if (!this.isForAsyncLoaderHookWorker ||
+        requestType === kRequireInImportedCJS || requestType === kImportInRequiredESM) {
       // On the main thread, always resolve synchronously;
       // `resolveSync` coordinates with the async loader hook worker if needed.
+      // On the async loader hook worker, require(esm) and require() in imported CJS
+      // must still be resolved synchronously.
       maybePromise = this.resolveSync(parentURL, request);
       assert(!isPromise(maybePromise));
     } else {

test/fixtures/module-hooks/require-esm-in-hooks-thread/dep.mjs

@@ -0,0 +1 @@
+export const value = 'dep:';

test/fixtures/module-hooks/require-esm-in-hooks-thread/esm-with-import.mjs

@@ -0,0 +1,4 @@
+import { sep } from 'node:path';
+import { value } from './dep.mjs';
+
+export const result = `${value}${typeof sep}`;

test/fixtures/module-hooks/require-esm-in-hooks-thread/hooks.mjs

@@ -0,0 +1,9 @@
+// Loaded on the module.register() hooks thread. It defines no hooks: require()-ing
+// an ES module with imports while the hooks module is being loaded is enough.
+import { createRequire } from 'node:module';
+
+const require = createRequire(import.meta.url);
+const { result } = require('./esm-with-import.mjs');
+if (result !== 'dep:string') {
+  throw new Error(`Unexpected result: ${result}`);
+}

test/module-hooks/test-async-loader-hooks-require-esm-in-hooks-thread.mjs

@@ -0,0 +1,23 @@
+// Test that require(esm) of a module with imports works while loading a module.register() hooks module.
+// Refs: https://github.com/nodejs/node/issues/66767
+import '../common/index.mjs';
+import { execPath } from 'node:process';
+import fixtures from '../common/fixtures.js';
+import { spawnSyncAndAssert } from '../common/child_process.js';
+
+spawnSyncAndAssert(
+  execPath,
+  [
+    '--no-warnings',
+    '--input-type=module',
+    '--eval',
+    `import { register } from 'node:module';
+    register(${JSON.stringify(fixtures.fileURL('module-hooks/require-esm-in-hooks-thread/hooks.mjs'))});
+    console.log('ok');`,
+  ],
+  {
+    stdout: 'ok',
+    stderr: '',
+    trim: true,
+  },
+);