esm: refactor dynamic modules

[MylesBorins]

This is a change from the ecmascript-modules fork.
There is no change to behavior and we would like to
upstream to reduce the delta between our repos.

Refs: nodejs/ecmascript-modules#9

[nodejs-github-bot]

@MylesBorins build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1671/pipeline

[devsnek]

i think this needs to be something like

const exports = this.exports;
module.modulePromise.then(({ reflect }) => {
  reflect.exports.default.set(exports);
});

testing locally atm

[devsnek]

@MylesBorins this passes:

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index df270db69d..23bd8badbb 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -623,23 +623,24 @@ Module.prototype.load = function(filename) {
   if (experimentalModules) {
     if (asyncESM === undefined) lazyLoadESM();
     const ESMLoader = asyncESM.ESMLoader;
-    const url = pathToFileURL(filename);
-    const urlString = `${url}`;
+    const url = `${pathToFileURL(filename)}`;
+    const module = ESMLoader.moduleMap.get(url);
+    // create module entry at load time to snapshot exports correctly
     const exports = this.exports;
-    if (ESMLoader.moduleMap.has(urlString) !== true) {
+    if (module !== undefined) { // called from cjs translator
+      module.reflect.onReady((reflect) => {
+        reflect.exports.default.set(exports);
+      });
+    } else { // preemptively cache
       ESMLoader.moduleMap.set(
-        urlString,
+        url,
         new ModuleJob(ESMLoader, url, async () => {
-          const ctx = createDynamicModule(
-            ['default'], url);
-          ctx.reflect.exports.default.set(exports);
-          return ctx;
+          return createDynamicModule(
+            ['default'], url, (reflect) => {
+              reflect.exports.default.set(exports);
+            });
         })
       );
-    } else {
-      const job = ESMLoader.moduleMap.get(urlString);
-      if (job.reflect)
-        job.reflect.exports.default.set(exports);
     }
   }
 };
diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js
index 8e93a08502..9afc866e40 100644
--- a/lib/internal/modules/esm/create_dynamic_module.js
+++ b/lib/internal/modules/esm/create_dynamic_module.js
@@ -1,6 +1,6 @@
 'use strict';

-const { ModuleWrap } = internalBinding('module_wrap');
+const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
 const debug = require('util').debuglog('esm');
 const ArrayJoin = Function.call.bind(Array.prototype.join);
 const ArrayMap = Function.call.bind(Array.prototype.map);
@@ -10,50 +10,47 @@ const createDynamicModule = (exports, url = '', evaluate) => {
     `creating ESM facade for ${url} with exports: ${ArrayJoin(exports, ', ')}`
   );
   const names = ArrayMap(exports, (name) => `${name}`);
-  // Create two modules: One whose exports are get- and set-able ('reflective'),
-  // and one which re-exports all of these but additionally may
-  // run an executor function once everything is set up.
-  const src = `
-  export let executor;
-  ${ArrayJoin(ArrayMap(names, (name) => `export let $${name};`), '\n')}
-  /* This function is implicitly returned as the module's completion value */
-  (() => ({
-    setExecutor: fn => executor = fn,
-    reflect: {
-      exports: { ${
-  ArrayJoin(ArrayMap(names, (name) => `
-        ${name}: {
-          get: () => $${name},
-          set: v => $${name} = v
-        }`), ', \n')}
-      }
-    }
-  }));`;
-  const reflectiveModule = new ModuleWrap(src, `cjs-facade:${url}`);
-  reflectiveModule.instantiate();
-  const { setExecutor, reflect } = reflectiveModule.evaluate(-1, false)();
-  // public exposed ESM
-  const reexports = `
-  import {
-    executor,
-    ${ArrayMap(names, (name) => `$${name}`)}
-  } from "";
-  export {
-    ${ArrayJoin(ArrayMap(names, (name) => `$${name} as ${name}`), ', ')}
-  }
-  if (typeof executor === "function") {
-    // add await to this later if top level await comes along
-    executor()
-  }`;
-  if (typeof evaluate === 'function') {
-    setExecutor(() => evaluate(reflect));
-  }
-  const module = new ModuleWrap(reexports, `${url}`);
-  module.link(async () => reflectiveModule);
-  module.instantiate();
-  reflect.namespace = module.namespace();
+
+  const source = `
+${ArrayJoin(ArrayMap(names, (name) =>
+    `let $${name};
+export { $${name} as ${name} };
+import.meta.exports.${name} = {
+  get: () => $${name},
+  set: (v) => $${name} = v,
+};`), '\n')
+}
+
+import.meta.done();
+`;
+
+  const m = new ModuleWrap(source, `${url}`);
+  m.link(() => 0);
+  m.instantiate();
+
+  const readyfns = new Set();
+  const reflect = {
+    namespace: m.namespace(),
+    exports: {},
+    onReady: (cb) => { readyfns.add(cb); },
+  };
+
+  callbackMap.set(m, {
+    initializeImportMeta: (meta, wrap) => {
+      meta.exports = reflect.exports;
+      meta.done = () => {
+        evaluate(reflect);
+        reflect.onReady = (cb) => cb(reflect);
+        for (const fn of readyfns) {
+          readyfns.delete(fn);
+          fn(reflect);
+        }
+      };
+    },
+  });
+
   return {
-    module,
+    module: m,
     reflect,
   };
 };
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
index 0c34283b8a..0d19a728aa 100644
--- a/lib/internal/modules/esm/translators.js
+++ b/lib/internal/modules/esm/translators.js
@@ -60,9 +60,10 @@ translators.set('cjs', async (url, isMain) => {
   const module = CJSModule._cache[
     isWindows ? StringReplace(pathname, winSepRegEx, '\\') : pathname];
   if (module && module.loaded) {
-    const ctx = createDynamicModule(['default'], url);
-    ctx.reflect.exports.default.set(module.exports);
-    return ctx;
+    const exports = module.exports;
+    return createDynamicModule(['default'], url, (reflect) => {
+      reflect.exports.default.set(exports);
+    });
   }
   return createDynamicModule(['default'], url, () => {
     debug(`Loading CJSModule ${url}`);

[MylesBorins]

I'm very confused as to why this patch failed on master but CI passed without an issue on ecma-script modules (which was recently rebased against master)

original pr: nodejs/ecmascript-modules#9
original ci: https://ci.nodejs.org/job/node-test-pull-request/18832/

[devsnek]

@MylesBorins because downstream removed all the important tests for it, because we removed interop.

[MylesBorins]

Updated to the newer patch you shared above

CI: https://ci.nodejs.org/job/node-test-pull-request/18867/

[MylesBorins]

landed in 2931c50