Change bindings priority so it matches Miniflare 1's

From lowest to highest priority:
1) Wrangler [vars]
2) .env Variables
3) WASM Module Bindings
4) Custom Bindings

Author: mrbbot

packages/cli-parser/src/help.ts

@@ -65,7 +65,7 @@ export function buildHelp<Plugins extends PluginSignatures>(
     for (const [key, meta] of entries) {
       const { type, typeFormat, alias, description = "", negatable } = meta;
       // Ignore API-only options (e.g. string script)
-      if (type === OptionType.NONE) continue;
+      if (type === OptionType.NONE || typeof key === "symbol") continue;
 
       let name = argName(key, meta);
       if (negatable) name = `(no-)${name}`;

packages/cli-parser/src/parse.ts

@@ -51,7 +51,7 @@ export function parseArgv<Plugins extends PluginSignatures>(
     for (const [key, meta] of plugin.prototype.opts.entries()) {
       const type = meta.type;
       // Ignore API-only options (e.g. string script)
-      if (type === OptionType.NONE) continue;
+      if (type === OptionType.NONE || typeof key === "symbol") continue;
 
       // Record arg so we can construct result options object later, note we're
       // including positional arguments here so they're included in the result

packages/core/src/index.ts

@@ -46,6 +46,23 @@ export * from "./plugins";
 export * from "./standards";
 export * from "./storage";
 
+/** @internal */
+// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
+export function _deepEqual(a: any, b: any): boolean {
+  if (!dequal(a, b)) return false;
+
+  // Check top-level symbol properties are equal (used by BindingsPlugin for
+  // Wrangler variables)
+  if (typeof a === "object") {
+    const aSymbols = Object.getOwnPropertySymbols(a);
+    for (const aSymbol of aSymbols) {
+      if (!(aSymbol in b) || !dequal(a[aSymbol], b[aSymbol])) return false;
+    }
+    return aSymbols.length === Object.getOwnPropertySymbols(b).length;
+  }
+  return true;
+}
+
 export type CorePluginSignatures = PluginSignatures & {
   CorePlugin: typeof CorePlugin;
 };
@@ -120,6 +137,7 @@ function splitWranglerConfig<Plugins extends PluginSignatures>(
     const pluginResult = {} as PluginOptionsUnion<Plugins>;
     const pluginOverrides = overrides[name];
     for (const [key, meta] of plugin.prototype.opts?.entries() ?? []) {
+      // TODO: merge options
       (pluginResult as any)[key] =
         pluginOverrides[key] ?? meta.fromWrangler?.(config, configDir);
     }
@@ -329,7 +347,7 @@ export class MiniflareCore<
       if (
         previous !== undefined &&
         !compatUpdate &&
-        dequal(previous[name], options[name])
+        _deepEqual(previous[name], options[name])
       ) {
         continue;
       }
@@ -353,7 +371,7 @@ export class MiniflareCore<
       if (
         previous !== undefined &&
         !compatUpdate &&
-        dequal(previous[name], options[name]) &&
+        _deepEqual(previous[name], options[name]) &&
         // Make sure if we ran any beforeSetups and this plugin previously
         // returned scripts, that we rerun its setup
         !(ranBeforeSetup && this.#setupResults.get(name)?.script)

packages/core/src/plugins/bindings.ts

@@ -11,6 +11,8 @@ import {
 } from "@miniflare/shared";
 import dotenv from "dotenv";
 
+const kWranglerBindings = Symbol("kWranglerBindings");
+
 export interface BindingsOptions {
   envPath?: boolean | string;
   bindings?: Record<string, any>;
@@ -36,11 +38,12 @@ export class BindingsPlugin
   })
   envPath?: boolean | string;
 
+  // We want custom bindings to override Wrangler bindings, so we can't put
+  // fromWrangler in `bindings`. Using a symbol, means these low-priority
+  // bindings can only be loaded from a Wrangler config.
   @Option({
     type: OptionType.OBJECT,
-    alias: "b",
-    description: "Binds variable/secret to environment",
-    logName: "Custom Bindings",
+    logName: "Wrangler Variables",
     fromWrangler: ({ vars }) => {
       if (!vars) return;
       // Wrangler stringifies all environment variables
@@ -49,6 +52,14 @@ export class BindingsPlugin
       );
     },
   })
+  [kWranglerBindings]?: Record<string, any>;
+
+  @Option({
+    type: OptionType.OBJECT,
+    alias: "b",
+    description: "Binds variable/secret to environment",
+    logName: "Custom Bindings",
+  })
   bindings?: Record<string, any>;
 
   @Option({
@@ -80,19 +91,17 @@ export class BindingsPlugin
   }
 
   async setup(): Promise<SetupResult> {
+    // Bindings should be loaded in this order, from lowest to highest priority:
+    // 1) Wrangler [vars]
+    // 2) .env Variables
+    // 3) WASM Module Bindings
+    // 4) Custom Bindings
+
     const bindings: Context = {};
     const watch: string[] = [];
 
-    // Load WebAssembly module bindings from files
-    if (this.wasmBindings) {
-      for (const [name, wasmPath] of Object.entries(this.wasmBindings)) {
-        bindings[name] = new WebAssembly.Module(await fs.readFile(wasmPath));
-        watch.push(wasmPath);
-      }
-    }
-
-    // Copy user's arbitrary bindings
-    Object.assign(bindings, this.bindings);
+    // Copy Wrangler bindings first
+    Object.assign(bindings, this[kWranglerBindings]);
 
     // Load bindings from .env file
     const envPath = this.envPath === true ? this.defaultEnvPath : this.envPath;
@@ -109,6 +118,17 @@ export class BindingsPlugin
       watch.push(envPath);
     }
 
+    // Load WebAssembly module bindings from files
+    if (this.wasmBindings) {
+      for (const [name, wasmPath] of Object.entries(this.wasmBindings)) {
+        bindings[name] = new WebAssembly.Module(await fs.readFile(wasmPath));
+        watch.push(wasmPath);
+      }
+    }
+
+    // Copy user's arbitrary bindings
+    Object.assign(bindings, this.bindings);
+
     return { globals: this.globals, bindings, watch };
   }
 }

packages/core/test/index.spec.ts

@@ -12,6 +12,7 @@ import {
   Request,
   RequestInfo,
   RequestInit,
+  _deepEqual,
 } from "@miniflare/core";
 import { VMScriptRunner } from "@miniflare/runner-vm";
 import {
@@ -51,6 +52,16 @@ function waitForReload(mf: MiniflareCore<any>): Promise<unknown> {
   return reloadPromise;
 }
 
+test("_deepEqual: checks top-level symbol property equality", (t) => {
+  const a = Symbol("a");
+  const b = Symbol("b");
+  t.true(_deepEqual({ [a]: { a: 1 } }, { [a]: { a: 1 } }));
+  t.false(_deepEqual({ [a]: { a: 1 } }, { [a]: { a: 2 } }));
+  t.false(_deepEqual({ [a]: { a: 1 } }, { [b]: { a: 1 } }));
+  t.false(_deepEqual({ [a]: { a: 1 } }, {}));
+  t.false(_deepEqual({}, { [a]: { a: 1 } }));
+});
+
 test("MiniflareCore: always loads CorePlugin first", async (t) => {
   const log = new TestLog();
   const ctx: MiniflareCoreContext = { log, storageFactory, scriptRunner };
@@ -229,15 +240,15 @@ test("MiniflareCore: #init: gets watching option once", async (t) => {
   const mf = useMiniflare(plugins, { wranglerConfigPath });
   let instances = await mf.getPlugins();
   t.true(instances.CorePlugin.watch);
-  t.is(instances.BindingsPlugin.bindings?.KEY, "1");
+  t.is((await instances.BindingsPlugin.setup()).bindings?.KEY, "1");
 
   // Disable watching in wrangler.toml
   let reloadPromise = waitForReload(mf);
   await fs.writeFile(wranglerConfigPath, config("2", false));
   await reloadPromise;
   instances = await mf.getPlugins();
   t.false(instances.CorePlugin.watch);
-  t.is(instances.BindingsPlugin.bindings?.KEY, "2");
+  t.is((await instances.BindingsPlugin.setup()).bindings?.KEY, "2");
 
   // Edit KEY again in wrangler.toml, file should still be watched even though
   // watching has been "disabled"
@@ -246,7 +257,7 @@ test("MiniflareCore: #init: gets watching option once", async (t) => {
   await reloadPromise;
   instances = await mf.getPlugins();
   t.false(instances.CorePlugin.watch);
-  t.is(instances.BindingsPlugin.bindings?.KEY, "3");
+  t.is((await instances.BindingsPlugin.setup()).bindings?.KEY, "3");
 });
 test("MiniflareCore: #init: creates all plugins on init", async (t) => {
   const log = new TestLog();
@@ -677,15 +688,15 @@ test("MiniflareCore: #watcherCallback: re-inits on wrangler config change", asyn
   );
   const plugins = await mf.getPlugins();
   const bindingsPlugin = plugins.BindingsPlugin;
-  t.is(bindingsPlugin.bindings?.KEY, "value1");
+  t.is((await bindingsPlugin.setup()).bindings?.KEY, "value1");
 
   // Update wrangler config, expecting BindingsPlugin to be re-created
   log.logs = [];
   const reloadPromise = waitForReload(mf);
   await fs.writeFile(wranglerConfigPath, '[vars]\nKEY = "value2"');
   await reloadPromise;
   t.not(plugins.BindingsPlugin, bindingsPlugin); // re-created
-  t.is(plugins.BindingsPlugin.bindings?.KEY, "value2");
+  t.is((await plugins.BindingsPlugin.setup()).bindings?.KEY, "value2");
   startsWith(t, log.logsAtLevel(LogLevel.DEBUG), [
     `${relative(wranglerConfigPath)} changed...`,
     "Initialising worker...", // re-init

packages/core/test/plugins/bindings.spec.ts

@@ -58,9 +58,8 @@ test("BindingsPlugin: parses options from argv", (t) => {
     bindings: { KEY1: "value1", KEY2: "value2" },
   });
 });
-test("BindingsPlugin: parses options from wrangler config", (t) => {
-  const options = parsePluginWranglerConfig(BindingsPlugin, {
-    vars: { KEY1: "value1", KEY2: "value2", KEY3: true, KEY4: 42 },
+test("BindingsPlugin: parses options from wrangler config", async (t) => {
+  let options = parsePluginWranglerConfig(BindingsPlugin, {
     wasm_modules: {
       MODULE1: "module1.wasm",
       MODULE2: "module2.wasm",
@@ -70,26 +69,45 @@ test("BindingsPlugin: parses options from wrangler config", (t) => {
       env_path: ".env.test",
     },
   });
-  t.deepEqual(options, {
+  t.like(options, {
     envPath: ".env.test",
-    // Bindings should be stringified...
-    bindings: { KEY1: "value1", KEY2: "value2", KEY3: "true", KEY4: "42" },
-    // ...but globals shouldn't
     globals: { KEY5: "value5", KEY6: false, KEY7: 10 },
     wasmBindings: { MODULE1: "module1.wasm", MODULE2: "module2.wasm" },
   });
+
+  // Wrangler bindings are stored in the kWranglerBindings symbol, which isn't
+  // exported, so setup the plugin and check they're included
+  options = parsePluginWranglerConfig(BindingsPlugin, {
+    vars: { KEY1: "value1", KEY2: "value2", KEY3: true, KEY4: 42 },
+  });
+  const log = new NoOpLog();
+  const plugin = new BindingsPlugin(log, compat, options);
+  const result = await plugin.setup();
+  // Wrangler bindings should be stringified
+  t.deepEqual(result.bindings, {
+    KEY1: "value1",
+    KEY2: "value2",
+    KEY3: "true",
+    KEY4: "42",
+  });
 });
 test("BindingsPlugin: logs options", (t) => {
+  // wranglerOptions should contain [kWranglerBindings]
+  const wranglerOptions = parsePluginWranglerConfig(BindingsPlugin, {
+    vars: { KEY1: "value1", KEY2: "value2" },
+  });
   let logs = logPluginOptions(BindingsPlugin, {
+    ...wranglerOptions,
     envPath: ".env.custom",
-    bindings: { KEY1: "value1", KEY2: "value2" },
-    globals: { KEY3: "value3", KEY4: "value4" },
+    bindings: { KEY3: "value3", KEY4: "value4" },
+    globals: { KEY5: "value5", KEY6: "value6" },
     wasmBindings: { MODULE1: "module1.wasm", MODULE2: "module2.wasm" },
   });
   t.deepEqual(logs, [
     "Env Path: .env.custom",
-    "Custom Bindings: KEY1, KEY2",
-    "Custom Globals: KEY3, KEY4",
+    "Wrangler Variables: KEY1, KEY2",
+    "Custom Bindings: KEY3, KEY4",
+    "Custom Globals: KEY5, KEY6",
     "WASM Bindings: MODULE1, MODULE2",
   ]);
   logs = logPluginOptions(BindingsPlugin, { envPath: true });
@@ -182,27 +200,37 @@ test("BindingsPlugin: setup: loads WebAssembly bindings", async (t) => {
 });
 test("BindingsPlugin: setup: loads bindings from all sources", async (t) => {
   const log = new NoOpLog();
+
+  // Bindings should be loaded in this order, from lowest to highest priority:
+  // 1) Wrangler [vars]
+  // 2) .env Variables
+  // 3) WASM Module Bindings
+  // 4) Custom Bindings
+
+  // wranglerOptions should contain [kWranglerBindings]
+  const wranglerOptions = parsePluginWranglerConfig(BindingsPlugin, {
+    vars: { A: "wrangler", B: "wrangler", C: "wrangler", D: "wrangler" },
+  });
+
   const tmp = await useTmp(t);
   const envPath = path.join(tmp, ".env");
-  await fs.writeFile(envPath, "C=c");
-  const obj = { c: 3 };
+  await fs.writeFile(envPath, "A=env\nB=env\nC=env");
+
+  const obj = { ping: "pong" };
   const plugin = new BindingsPlugin(log, compat, {
+    ...wranglerOptions,
     wasmBindings: {
       A: addModulePath,
       B: addModulePath,
-      C: addModulePath,
     },
-    bindings: { B: obj, C: obj },
+    bindings: { A: obj },
     envPath,
   });
   const result = await plugin.setup();
   assert(result.bindings);
 
-  const instance = new WebAssembly.Instance(result.bindings.A);
-  assert(typeof instance.exports.add === "function");
-  t.is(instance.exports.add(1, 2), 3);
-
-  t.is(result.bindings.B, obj);
-
-  t.is(result.bindings.C, "c");
+  t.is(result.bindings.D, "wrangler");
+  t.is(result.bindings.C, "env");
+  t.true(result.bindings.B instanceof WebAssembly.Module);
+  t.is(result.bindings.A, obj);
 });

packages/shared/src/plugin.ts

@@ -6,7 +6,7 @@ import { StorageFactory } from "./storage";
 import { Awaitable } from "./sync";
 import { WranglerConfig } from "./wrangler";
 
-export type Context = { [key: string]: any };
+export type Context = { [key: string | symbol]: any };
 
 export enum OptionType {
   NONE, // never
@@ -51,9 +51,12 @@ export type OptionMetadata =
 
 export function Option(
   metadata: OptionMetadata
-): (prototype: typeof Plugin.prototype, key: string) => void {
+): (prototype: typeof Plugin.prototype, key: string | symbol) => void {
   return function (prototype, key) {
-    (prototype.opts ??= new Map<string, OptionMetadata>()).set(key, metadata);
+    (prototype.opts ??= new Map<string | symbol, OptionMetadata>()).set(
+      key,
+      metadata
+    );
   };
 }
 
@@ -72,7 +75,7 @@ export abstract class Plugin<Options extends Context = never> {
   // noinspection JSUnusedLocalSymbols
   readonly #phantom!: Options;
   // Metadata added by @Option decorator
-  opts?: Map<string, OptionMetadata>;
+  opts?: Map<string | symbol, OptionMetadata>;
 
   protected constructor(
     protected readonly log: Log,
@@ -106,7 +109,7 @@ export abstract class Plugin<Options extends Context = never> {
 
 export type PluginSignature = {
   new (log: Log, compat: Compatibility, options?: Context): Plugin<Context>;
-  prototype: { opts?: Map<string, OptionMetadata> };
+  prototype: { opts?: Map<string | symbol, OptionMetadata> };
 };
 export type PluginSignatures = { [pluginName: string]: PluginSignature };
 
@@ -157,7 +160,8 @@ export function logOptions<Plugins extends PluginSignatures>(
     for (const [key, meta] of plugin.prototype.opts?.entries() ?? []) {
       const value = pluginOptions[key];
       if (value === undefined || meta.type === OptionType.NONE) continue;
-      const keyName = meta?.logName ?? titleCase(key);
+      const keyName =
+        meta?.logName ?? titleCase(typeof key === "symbol" ? "<symbol>" : key);
       let str: string | undefined;
       if (meta.logValue) {
         str = (meta.logValue as any)(value);