Tidy up service binding name/binding warnings, ref #302

Author: mrbbot

packages/core/src/plugins/bindings.ts

@@ -4,6 +4,7 @@ import path from "path";
 import {
   Awaitable,
   Context,
+  Log,
   Mount,
   Option,
   OptionType,
@@ -120,6 +121,40 @@ export class Fetcher {
   }
 }
 
+function getServiceBindingName(
+  log: Log,
+  service: string,
+  { name, binding }: Partial<WranglerServiceConfig>
+) {
+  // 1. Make sure name and binding match if both defined
+  if (name !== undefined && binding !== undefined && name !== binding) {
+    throw new MiniflareCoreError(
+      "ERR_SERVICE_NAME_MISMATCH",
+      `Service "${service}" declared with \`name\`="${name}" and \`binding\`="${binding}".
+The \`binding\` key should be used to define binding names.`
+    );
+  }
+
+  // 2. If name is defined, use it but log a warning
+  if (name !== undefined) {
+    log.warn(
+      `Service "${service}" declared using deprecated syntax.
+The \`name\` key should be removed and renamed to \`binding\`.`
+    );
+    return name;
+  }
+
+  // 3. If binding is defined, use it
+  if (binding !== undefined) return binding;
+
+  // 4. Otherwise, neither `name` nor `binding` defined, so throw
+  throw new MiniflareCoreError(
+    "ERR_SERVICE_NO_NAME",
+    `Service "${service}" declared with neither \`binding\` nor \`name\` keys.
+The \`binding\` key should be used to define binding names.`
+  );
+}
+
 export class BindingsPlugin
   extends Plugin<BindingsOptions>
   implements BindingsOptions
@@ -236,42 +271,16 @@ export class BindingsPlugin
     fromWrangler: ({ services, experimental_services }, configDir, log) => {
       if (experimental_services) {
         log.warn(
-          "Using `experimental_services` in `wrangler.toml` is deprecated. " +
+          "Using the `experimental_services` key is deprecated. " +
             "This key should be renamed to `services`."
         );
       }
       const allServices: WranglerServiceConfig[] = [];
       if (services) allServices.push(...services);
       if (experimental_services) allServices.push(...experimental_services);
-      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 })] = {
+          services[getServiceBindingName(log, service, { name, binding })] = {
             service,
             environment,
           };

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

@@ -191,11 +191,11 @@ test("BindingsPlugin: logs warning if `name` is used instead of `binding`", asyn
   t.true(warning.includes(service));
   t.regex(
     warning,
-    /Service "\w+" is declared using deprecated syntax. Key `name` should be renamed to `binding`./
+    /^Service "\w+" declared using deprecated syntax\.\nThe `name` key should be removed and renamed to `binding`\.$/
   );
 });
 
-test("BindingsPlugin: logs no warning if `name` and `binding` are both used but they are the same", async (t) => {
+test("BindingsPlugin: logs 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";
@@ -212,7 +212,14 @@ test("BindingsPlugin: logs no warning if `name` and `binding` are both used but
 
   // Check no warnings logged
   const warnings = log.logsAtLevel(LogLevel.WARN);
-  t.is(warnings.length, 0);
+  t.is(warnings.length, 1);
+  const [warning] = warnings;
+
+  t.true(warning.includes(service));
+  t.regex(
+    warning,
+    /^Service "\w+" declared using deprecated syntax\.\nThe `name` key should be removed and renamed to `binding`\.$/
+  );
 });
 
 test("BindingsPlugin: throws if `name` and `binding` are both present and don't match", (t) => {