Fix inconsistency: services have binding not name in wrangler.toml (#302

)

* Allow helper `parsePluginWranglerConfig` to be given `Log` instances

* Fix inconsistency: services have `binding` not `name` in `wrangler.toml`

I noticed that in Workers docs and Miniflare there's inconsistency in how
services are declared:

- Workers expect `binding` key
https://github.com/cloudflare/cloudflare-docs/blob/a65a35843ffb7b69caf2758cc5f2ad805251d166/content/workers/wrangler/configuration.md?plain=1#L212-L220

- Miniflare expects `name`

I added 2 graceful and 2 error scenarios:
- (graceful) accept `name` with warning if `binding` is missing
- (graceful) accept `name` and `binding` both given and the same
- (error) throw if both `binding` and `name` are given but they don't match
- (error) throw if neither `binding` not `name` is given

I decided to handle it with errors, not with mutually exclusive presence
of either `name` or `binding` in interface `WranglerServiceConfig`
because `name` should go away - it's just a behavior mismatch.

Related: #280
Syntax in wrangler2: cloudflare/workers-sdk#906

packages/core/src/error.ts

@@ -7,6 +7,8 @@ export type MiniflareCoreErrorCode =
   | "ERR_MOUNT" // Error whilst mounting worker
   | "ERR_MOUNT_NAME_MISMATCH" // Mounted name must match service name if defined
   | "ERR_SERVICE_NOT_MOUNTED" // Mount for service binding not found
+  | "ERR_SERVICE_NO_NAME" // In service definition in wranger.toml three's no `binding` (and no deprecated `name` either)
+  | "ERR_SERVICE_NAME_MISMATCH" // In service definition in wranger.toml three's both `name` and `binding` and they don't match
   | "ERR_INVALID_UPSTREAM"; // Invalid upstream URL
 
 export class MiniflareCoreError extends MiniflareError<MiniflareCoreErrorCode> {}

packages/core/src/plugins/bindings.ts

@@ -243,10 +243,42 @@ export class BindingsPlugin
       const allServices: WranglerServiceConfig[] = [];
       if (services) allServices.push(...services);
       if (experimental_services) allServices.push(...experimental_services);
-      return allServices?.reduce((services, { name, service, environment }) => {
-        services[name] = { service, environment };
-        return services;
-      }, {} as ServiceBindingsOptions);
+      const getBindingName = (
+        service: string,
+        { name, binding }: Partial<WranglerServiceConfig>
+      ): string => {
+        if (name && binding && name !== binding) {
+          throw new MiniflareCoreError(
+            "ERR_SERVICE_NAME_MISMATCH",
+            `Service "${service}" declared with name=${name} and binding=${binding}.
+            \`binding\` key should be used to define binding names.`
+          );
+        } else if (binding === undefined) {
+          if (name) {
+            log.warn(
+              `Service "${service}" is declared using deprecated syntax. Key \`name\` should be renamed to \`binding\`.`
+            );
+            return name;
+          }
+          throw new MiniflareCoreError(
+            "ERR_SERVICE_NO_NAME",
+            `Service "${service}" declared with neither \`bindding\` nor \`name\`.
+            \`binding\` key should be used to define binding names.`
+          );
+        }
+
+        return binding;
+      };
+      return allServices?.reduce(
+        (services, { name, binding, service, environment }) => {
+          services[getBindingName(service, { name, binding })] = {
+            service,
+            environment,
+          };
+          return services;
+        },
+        {} as ServiceBindingsOptions
+      );
     },
   })
   serviceBindings?: ServiceBindingsOptions;

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

@@ -13,11 +13,13 @@ import {
 } from "@miniflare/core";
 import {
   Compatibility,
+  LogLevel,
   NoOpLog,
   PluginContext,
   getRequestContext,
   viewToBuffer,
 } from "@miniflare/shared";
+import { TestLog } from "@miniflare/shared-test";
 import {
   getObjectProperties,
   logPluginOptions,
@@ -124,9 +126,12 @@ test("BindingsPlugin: parses options from wrangler config", async (t) => {
     services: [
       { name: "SERVICE1", service: "service1", environment: "development" },
       { name: "SERVICE2", service: "service2", environment: "production" },
+      { binding: "SERVICE_A", service: "service1", environment: "development" },
+      { binding: "SERVICE_B", service: "service2", environment: "production" },
     ],
     experimental_services: [
       { name: "SERVICE3", service: "service3", environment: "staging" },
+      { binding: "SERVICE_C", service: "service3", environment: "staging" },
     ],
     miniflare: {
       globals: { KEY5: "value5", KEY6: false, KEY7: 10 },
@@ -143,6 +148,9 @@ test("BindingsPlugin: parses options from wrangler config", async (t) => {
       SERVICE1: { service: "service1", environment: "development" },
       SERVICE2: { service: "service2", environment: "production" },
       SERVICE3: { service: "service3", environment: "staging" },
+      SERVICE_A: { service: "service1", environment: "development" },
+      SERVICE_B: { service: "service2", environment: "production" },
+      SERVICE_C: { service: "service3", environment: "staging" },
     },
   });
 
@@ -161,6 +169,90 @@ test("BindingsPlugin: parses options from wrangler config", async (t) => {
     KEY4: "42",
   });
 });
+
+test("BindingsPlugin: logs warning if `name` is used instead of `binding`", async (t) => {
+  const log = new TestLog();
+  const service = "service123";
+
+  parsePluginWranglerConfig(
+    BindingsPlugin,
+    {
+      services: [{ name: "SERVICE123", service, environment: "development" }],
+    },
+    "",
+    log
+  );
+
+  // Check warning logged
+  const warnings = log.logsAtLevel(LogLevel.WARN);
+  t.is(warnings.length, 1);
+  const [warning] = warnings;
+
+  t.true(warning.includes(service));
+  t.regex(
+    warning,
+    /Service "\w+" is declared using deprecated syntax. Key `name` should be renamed to `binding`./
+  );
+});
+
+test("BindingsPlugin: logs no warning if `name` and `binding` are both used but they are the same", async (t) => {
+  const log = new TestLog();
+  const service = "service123";
+  const name = "SERVICE123";
+  const binding = name;
+
+  parsePluginWranglerConfig(
+    BindingsPlugin,
+    {
+      services: [{ name, binding, service, environment: "development" }],
+    },
+    "",
+    log
+  );
+
+  // Check no warnings logged
+  const warnings = log.logsAtLevel(LogLevel.WARN);
+  t.is(warnings.length, 0);
+});
+
+test("BindingsPlugin: throws if `name` and `binding` are both present and don't match", (t) => {
+  t.throws(
+    () =>
+      parsePluginWranglerConfig(BindingsPlugin, {
+        services: [
+          {
+            name: "SERVICE1",
+            binding: "SERVICE_A",
+            service: "service1",
+            environment: "development",
+          },
+        ],
+      }),
+    {
+      instanceOf: MiniflareCoreError,
+      code: "ERR_SERVICE_NAME_MISMATCH",
+    }
+  );
+});
+
+test("BindingsPlugin: throws if `name` and `binding` are both absent", (t) => {
+  t.throws(
+    () =>
+      parsePluginWranglerConfig(BindingsPlugin, {
+        services: [
+          {
+            service: "service1",
+            environment: "development",
+          },
+        ],
+      }),
+    {
+      instanceOf: MiniflareCoreError,
+      code: "ERR_SERVICE_NO_NAME",
+    }
+  );
+});
+
 test("BindingsPlugin: logs options", (t) => {
   // wranglerOptions should contain [kWranglerBindings]
   const wranglerOptions = parsePluginWranglerConfig(BindingsPlugin, {

packages/shared-test/src/plugin.ts

@@ -3,6 +3,7 @@ import {
   BeforeSetupResult,
   Context,
   ExtractOptions,
+  Log,
   Mount,
   NoOpLog,
   Option,
@@ -27,15 +28,12 @@ export function parsePluginArgv<Plugin extends PluginSignature>(
 export function parsePluginWranglerConfig<Plugin extends PluginSignature>(
   plugin: Plugin,
   config: WranglerConfig,
-  configDir = ""
+  configDir = "",
+  log: Log = new NoOpLog()
 ): ExtractOptions<InstanceType<Plugin>> {
   const result = {} as ExtractOptions<InstanceType<Plugin>>;
   for (const [key, meta] of plugin.prototype.opts?.entries() ?? []) {
-    (result as any)[key] = meta.fromWrangler?.(
-      config,
-      configDir,
-      new NoOpLog()
-    );
+    (result as any)[key] = meta.fromWrangler?.(config, configDir, log);
   }
   return result;
 }

packages/shared/src/wrangler.ts

@@ -6,7 +6,9 @@ import { ModuleRuleType } from "./runner";
 export type UsageModel = "bundled" | "unbound";
 
 export interface WranglerServiceConfig {
-  name: string;
+  /** @deprecated Use `binding` instead */
+  name?: string;
+  binding?: string;
   service: string;
   environment: string;
 }