Implement export_commonjs_default/export_commonjs_namespace flags (#468)

* Implement `export_commonjs_default`/`export_commonjs_namespace` flags

Previously, the Workers runtime would incorrectly return
`{ default: module.exports }` from `require()`, as opposed to just
`module.exports`. The `export_commonjs_default` compatibility flag
enables the correct behaviour.

Miniflare previously implemented `export_commonjs_default` behaviour
for `CommonJS` modules, but `export_commonjs_namespace` behaviour for
all other types. This change switches everything to the correct
`export_commonjs_default` by default, but allows old behaviour to be
enabled by setting `export_commonjs_namespace`.

Note, importing `CommonJS` from an `ESModule` is not affected.

Ref: https://developers.cloudflare.com/workers/platform/compatibility-dates/#commonjs-modules-do-not-export-a-module-namespace

* fixup! Implement `export_commonjs_default`/`export_commonjs_namespace` flags

Author: mrbbot

packages/core/src/index.ts

@@ -776,7 +776,8 @@ export class MiniflareCore<
         globalScope,
         script,
         rules,
-        additionalModules
+        additionalModules,
+        this.#compat
       );
 
       this.#scriptWatchPaths.clear();

packages/runner-vm/src/index.ts

@@ -1,6 +1,7 @@
 import vm from "vm";
 import {
   AdditionalModules,
+  Compatibility,
   Context,
   ProcessedModuleRule,
   ScriptBlueprint,
@@ -44,7 +45,8 @@ export class VMScriptRunner implements ScriptRunner {
     globalScope: Context,
     blueprint: ScriptBlueprint,
     modulesRules?: ProcessedModuleRule[],
-    additionalModules?: AdditionalModules
+    additionalModules?: AdditionalModules,
+    compat?: Compatibility
   ): Promise<ScriptRunnerResult> {
     // If we're using modules, make sure --experimental-vm-modules is enabled
     if (modulesRules && !("SourceTextModule" in vm)) {
@@ -55,7 +57,8 @@ export class VMScriptRunner implements ScriptRunner {
     }
     // Also build a linker if we're using modules
     const linker =
-      modulesRules && new ModuleLinker(modulesRules, additionalModules ?? {});
+      modulesRules &&
+      new ModuleLinker(modulesRules, additionalModules ?? {}, compat);
 
     let context = this.context;
     if (context) {

packages/runner-vm/src/linker.ts

@@ -4,6 +4,7 @@ import path from "path";
 import vm, { SourceTextModuleImportModuleDynamically } from "vm";
 import {
   AdditionalModules,
+  Compatibility,
   Context,
   ProcessedModuleRule,
   STRING_SCRIPT_PATH,
@@ -32,10 +33,14 @@ export class ModuleLinker {
   readonly #referencedPathSizes = new Map<string, number>();
   readonly #moduleCache = new Map<string, vm.Module>();
   readonly #cjsModuleCache = new Map<string, CommonJSModule>();
+  // Make sure we always return the same objects from `require()` when the
+  // `export_commonjs_default` compatibility flag is disabled
+  readonly #namespaceCache = new WeakMap<any, any>();
 
   constructor(
-    private moduleRules: ProcessedModuleRule[],
-    private additionalModules: AdditionalModules
+    private readonly moduleRules: ProcessedModuleRule[],
+    private readonly additionalModules: AdditionalModules,
+    private readonly compat?: Compatibility
   ) {}
 
   get referencedPaths(): IterableIterator<string> {
@@ -110,7 +115,7 @@ export class ModuleLinker {
         });
         break;
       case "CommonJS":
-        const exports = this.loadCommonJSModule(
+        const mod = this.loadCommonJSModule(
           errorBase,
           identifier,
           spec,
@@ -119,7 +124,7 @@ export class ModuleLinker {
         module = new vm.SyntheticModule<{ default: Context }>(
           ["default"],
           function () {
-            this.setExport("default", exports);
+            this.setExport("default", mod.exports);
           },
           moduleOptions
         );
@@ -190,20 +195,20 @@ export class ModuleLinker {
     identifier: string,
     spec: string,
     context: vm.Context
-  ): any {
+  ): CommonJSModule {
     // If we've already seen a module with the same identifier, return it, to
     // handle import cycles
     const cached = this.#cjsModuleCache.get(identifier);
-    if (cached) return cached.exports;
+    if (cached) return cached;
 
     const additionalModule = this.additionalModules[spec];
     const module: CommonJSModule = { exports: {} };
 
     // If this is an additional module, return it immediately
     if (additionalModule) {
-      module.exports.default = additionalModule.default;
+      module.exports = additionalModule.default;
       this.#cjsModuleCache.set(identifier, module);
-      return module.exports;
+      return module;
     }
 
     const rule = this.moduleRules.find((rule) => rule.include.test(identifier));
@@ -242,21 +247,21 @@ export class ModuleLinker {
         moduleWrapper(module.exports, require, module);
         break;
       case "Text":
-        module.exports.default = data.toString("utf8");
+        module.exports = data.toString("utf8");
         break;
       case "Data":
-        module.exports.default = viewToBuffer(data);
+        module.exports = viewToBuffer(data);
         break;
       case "CompiledWasm":
-        module.exports.default = new WebAssembly.Module(data);
+        module.exports = new WebAssembly.Module(data);
         break;
       default:
         throw new VMScriptRunnerError(
           "ERR_MODULE_UNSUPPORTED",
           `${errorBase}: ${rule.type} modules are unsupported`
         );
     }
-    return module.exports;
+    return module;
   }
 
   private createRequire(referencingIdentifier: string, context: vm.Context) {
@@ -266,7 +271,24 @@ export class ModuleLinker {
       const errorBase = `Unable to resolve "${relative}" dependency "${spec}"`;
       // Get path to specified module relative to referencing module
       const identifier = path.resolve(referencingDirname, spec);
-      return this.loadCommonJSModule(errorBase, identifier, spec, context);
+      const mod = this.loadCommonJSModule(errorBase, identifier, spec, context);
+      // https://developers.cloudflare.com/workers/platform/compatibility-dates/#commonjs-modules-do-not-export-a-module-namespace
+      if (
+        this.compat === undefined ||
+        this.compat.isEnabled("export_commonjs_default")
+      ) {
+        return mod.exports;
+      } else {
+        // Make sure we always return the same object for an identifier
+        let ns = this.#namespaceCache.get(mod);
+        if (ns !== undefined) return ns;
+        ns = Object.defineProperty({}, "default", {
+          get: () => mod.exports,
+          enumerable: true,
+        });
+        this.#namespaceCache.set(mod, ns);
+        return ns;
+      }
     };
   }
 }

packages/runner-vm/test/fixtures/cjsadditional.cjs

@@ -1,2 +1,2 @@
 const additional = require("ADDITIONAL");
-module.exports = `CommonJS ${additional.default}`;
+module.exports = `CommonJS ${additional}`;

packages/runner-vm/test/fixtures/cjscompiledwasm.cjs

@@ -1,3 +1,3 @@
 const addModule = require("./add.wasm");
-const instance = new WebAssembly.Instance(addModule.default);
+const instance = new WebAssembly.Instance(addModule);
 exports.add1 = (a) => instance.exports.add(a, 1);

packages/runner-vm/test/fixtures/cjsdata.cjs

@@ -1,2 +1,2 @@
 const data = require("./data.bin");
-module.exports = `CommonJS ${new TextDecoder().decode(data.default).trimEnd()}`;
+module.exports = `CommonJS ${new TextDecoder().decode(data).trimEnd()}`;

packages/runner-vm/test/fixtures/cjsnamespace.cjs

@@ -0,0 +1,6 @@
+const cjs = require("./cjs.cjs");
+const txt = require("./text.txt");
+const txt2 = require("./text.txt");
+module.exports = function () {
+  return { cjs, txt, txt2 };
+};

packages/runner-vm/test/fixtures/cjstext.cjs

@@ -1,2 +1,2 @@
 const text = require("./text.txt");
-module.exports = `CommonJS ${text.default.trimEnd()}`;
+module.exports = `CommonJS ${text.trimEnd()}`;

packages/runner-vm/test/linker.spec.ts

@@ -4,6 +4,7 @@ import path from "path";
 import { TextDecoder } from "util";
 import { VMScriptRunner, VMScriptRunnerError } from "@miniflare/runner-vm";
 import {
+  Compatibility,
   Context,
   ModuleRule,
   ProcessedModuleRule,
@@ -35,9 +36,16 @@ const processedModuleRules = moduleRules.map<ProcessedModuleRule>((rule) => ({
 
 async function run(
   code: string,
-  globalScope: Context = {}
+  globalScope: Context = {},
+  compat?: Compatibility
 ): Promise<ScriptRunnerResult> {
-  return runner.run(globalScope, { code, filePath }, processedModuleRules);
+  return runner.run(
+    globalScope,
+    { code, filePath },
+    processedModuleRules,
+    undefined,
+    compat
+  );
 }
 
 test("ModuleLinker: links ESModule module via ES module", async (t) => {
@@ -310,3 +318,37 @@ test("ModuleLinker: permits dynamic import of statically linked module", async (
     dynamic: "ESModule test",
   });
 });
+
+test("ModuleLinker: respects export_commonjs_namespace compatibility flag", async (t) => {
+  let compat = new Compatibility(undefined, ["export_commonjs_default"]);
+  let result = await run(
+    `
+    import ns from "./cjsnamespace.cjs";
+    export default function() {
+      return ns;
+    }  
+    `,
+    undefined,
+    compat
+  );
+  let exports = await result.exports.default()();
+  t.is(exports.cjs, "CommonJS test");
+  t.is(exports.txt.trimEnd(), "Text test");
+  t.is(exports.txt, exports.txt2);
+
+  compat = new Compatibility(undefined, ["export_commonjs_namespace"]);
+  result = await run(
+    `
+    import ns from "./cjsnamespace.cjs";
+    export default function() {
+      return ns;
+    }  
+    `,
+    undefined,
+    compat
+  );
+  exports = await result.exports.default()();
+  t.is(exports.cjs.default, "CommonJS test");
+  t.is(exports.txt.default.trimEnd(), "Text test");
+  t.is(exports.txt, exports.txt2);
+});

packages/shared/src/compat.ts

@@ -14,6 +14,7 @@ export type CompatibilityEnableFlag =
   | "nodejs_compat"
   | "streams_enable_constructors"
   | "transformstream_enable_standard_constructor"
+  | "export_commonjs_default"
   | "r2_list_honor_include"
   | "global_navigator"
   | "durable_object_fetch_requires_full_url"
@@ -23,6 +24,7 @@ export type CompatibilityEnableFlag =
 export type CompatibilityDisableFlag =
   | "streams_disable_constructors"
   | "transformstream_disable_standard_constructor"
+  | "export_commonjs_namespace"
   | "no_global_navigator"
   | "durable_object_fetch_allows_relative_url"
   | "fetch_treats_unknown_protocols_as_http"
@@ -45,6 +47,11 @@ const FEATURES: CompatibilityFeature[] = [
     enableFlag: "transformstream_enable_standard_constructor",
     disableFlag: "transformstream_disable_standard_constructor",
   },
+  {
+    defaultAsOf: "2022-10-31",
+    enableFlag: "export_commonjs_default",
+    disableFlag: "export_commonjs_namespace",
+  },
   {
     defaultAsOf: "2022-08-04",
     enableFlag: "r2_list_honor_include",

packages/shared/src/runner.ts

@@ -1,3 +1,4 @@
+import { Compatibility } from "./compat";
 import { Matcher } from "./data";
 import { AdditionalModules, Context } from "./plugin";
 
@@ -38,6 +39,7 @@ export interface ScriptRunner {
     globalScope: Context,
     blueprint: ScriptBlueprint,
     modulesRules?: ProcessedModuleRule[],
-    additionalModules?: AdditionalModules
+    additionalModules?: AdditionalModules,
+    compat?: Compatibility
   ): Promise<ScriptRunnerResult>;
 }