fix: config.site.entry-point as a breaking deprecation (#577)

This makes configuring `site.entry-point` in config as a breaking deprecation, and throws an error. We do this because existing apps with `site.entry-point` _won't_ work in v2.
cloudflare · Mar 13, 2022 · 7faf0eb · 7faf0eb
1 parent c56847c
commit 7faf0eb
Show file tree

Hide file tree

Showing 8 changed files with 133 additions and 111 deletions.
diff --git a/.changeset/small-jokes-lay.md b/.changeset/small-jokes-lay.md
@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: `config.site.entry-point` as a breaking deprecation
+
+This makes configuring `site.entry-point` in config as a breaking deprecation, and throws an error. We do this because existing apps with `site.entry-point` _won't_ work in v2.
diff --git a/packages/wrangler/src/__tests__/configuration.test.ts b/packages/wrangler/src/__tests__/configuration.test.ts
@@ -254,74 +254,105 @@ describe("normalizeAndValidateConfig()", () => {
         `);
     });
 
-    it("should override `site` config defaults with provided values", () => {
-      const expectedConfig: RawConfig = {
-        site: {
-          bucket: "BUCKET",
-          "entry-point": "ENTRY_POINT",
-          include: ["INCLUDE_1", "INCLUDE_2"],
-          exclude: ["EXCLUDE_1", "EXCLUDE_2"],
-        },
-      };
+    describe("site", () => {
+      it("should override `site` config defaults with provided values", () => {
+        const expectedConfig: RawConfig = {
+          site: {
+            bucket: "BUCKET",
+            include: ["INCLUDE_1", "INCLUDE_2"],
+            exclude: ["EXCLUDE_1", "EXCLUDE_2"],
+          },
+        };
 
-      const { config, diagnostics } = normalizeAndValidateConfig(
-        expectedConfig,
-        undefined
-      );
+        const { config, diagnostics } = normalizeAndValidateConfig(
+          expectedConfig,
+          undefined
+        );
 
-      expect(config).toEqual(expect.objectContaining(expectedConfig));
-      expect(diagnostics.hasErrors()).toBe(false);
-      expect(diagnostics.hasWarnings()).toBe(false);
-    });
+        expect(config).toEqual(expect.objectContaining(expectedConfig));
+        expect(diagnostics.hasErrors()).toBe(false);
+        expect(diagnostics.hasWarnings()).toBe(false);
+      });
 
-    it("should error if `site` config is missing `bucket`", () => {
-      const expectedConfig: RawConfig = {
-        site: {
-          "entry-point": "ENTRY_POINT",
-          include: ["INCLUDE_1", "INCLUDE_2"],
-          exclude: ["EXCLUDE_1", "EXCLUDE_2"],
-        },
-      } as RawConfig;
+      it("should error if `site` config is missing `bucket`", () => {
+        const expectedConfig: RawConfig = {
+          // @ts-expect-error we're intentionally passing an invalid configuration here
+          site: {
+            include: ["INCLUDE_1", "INCLUDE_2"],
+            exclude: ["EXCLUDE_1", "EXCLUDE_2"],
+          },
+        };
 
-      const { config, diagnostics } = normalizeAndValidateConfig(
-        expectedConfig,
-        undefined
-      );
+        const { config, diagnostics } = normalizeAndValidateConfig(
+          expectedConfig,
+          undefined
+        );
 
-      expect(config).toEqual(expect.objectContaining(expectedConfig));
-      expect(diagnostics.hasErrors()).toBe(true);
-      expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
-                "Processing wrangler configuration:
-                  - ERROR: \\"site.bucket\\" is a required field."
-            `);
-      expect(diagnostics.hasWarnings()).toBe(false);
-    });
+        expect(config).toEqual(expect.objectContaining(expectedConfig));
+        expect(diagnostics.hasErrors()).toBe(true);
+        expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
+                  "Processing wrangler configuration:
+                    - ERROR: \\"site.bucket\\" is a required field."
+              `);
+        expect(diagnostics.hasWarnings()).toBe(false);
+      });
 
-    it("should error on invalid `site` values", () => {
-      const expectedConfig = {
-        site: {
-          bucket: "BUCKET",
-          "entry-point": 111,
-          include: [222, 333],
-          exclude: [444, 555],
-        },
-      };
+      it("should error on invalid `site` values", () => {
+        const expectedConfig = {
+          site: {
+            bucket: "BUCKET",
+            include: [222, 333],
+            exclude: [444, 555],
+          },
+        };
 
-      const { config, diagnostics } = normalizeAndValidateConfig(
-        expectedConfig as unknown as RawConfig,
-        undefined
-      );
+        const { config, diagnostics } = normalizeAndValidateConfig(
+          expectedConfig as unknown as RawConfig,
+          undefined
+        );
 
-      expect(config).toEqual(expect.objectContaining(expectedConfig));
-      expect(diagnostics.hasWarnings()).toBe(false);
-      expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
+        expect(config).toEqual(expect.objectContaining(expectedConfig));
+        expect(diagnostics.hasWarnings()).toBe(false);
+        expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
+            "Processing wrangler configuration:
+              - ERROR: \\"sites.include.[0]\\" should be of type string but got 222.
+              - ERROR: \\"sites.include.[1]\\" should be of type string but got 333.
+              - ERROR: \\"sites.exclude.[0]\\" should be of type string but got 444.
+              - ERROR: \\"sites.exclude.[1]\\" should be of type string but got 555."
+          `);
+      });
+
+      it("should error with a deprecation warning if entry-point is defined", async () => {
+        const { config, diagnostics } = normalizeAndValidateConfig(
+          {
+            site: {
+              bucket: "some/path",
+              "entry-point": "some/other/script.js",
+            },
+          } as unknown as RawConfig,
+          undefined
+        );
+
+        expect(config.site).toMatchInlineSnapshot(`
+          Object {
+            "bucket": "some/path",
+            "exclude": Array [],
+            "include": Array [],
+          }
+        `);
+        expect(diagnostics.hasWarnings()).toBe(true);
+        expect(diagnostics.hasErrors()).toBe(true);
+        expect(diagnostics.renderWarnings()).toMatchInlineSnapshot(`
           "Processing wrangler configuration:
-            - ERROR: \\"site.entry_point\\" should be of type string but got 111.
-            - ERROR: \\"sites.include.[0]\\" should be of type string but got 222.
-            - ERROR: \\"sites.include.[1]\\" should be of type string but got 333.
-            - ERROR: \\"sites.exclude.[0]\\" should be of type string but got 444.
-            - ERROR: \\"sites.exclude.[1]\\" should be of type string but got 555."
+            - Unexpected fields found in site field: \\"entry-point\\""
         `);
+        expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
+          "Processing wrangler configuration:
+            - DEPRECATION: \\"site.entry-point\\":
+              The \`site.entry-point\` config field is no longer used.
+              The entry-point should be specified via the command line or the \`main\` config field."
+        `);
+      });
     });
 
     it("should map `wasm_module` paths from relative to the config path to relative to the cwd", () => {

diff --git a/packages/wrangler/src/__tests__/publish.test.ts b/packages/wrangler/src/__tests__/publish.test.ts
@@ -13,7 +13,6 @@ import { mockKeyListRequest } from "./helpers/mock-kv";
 import { runInTempDir } from "./helpers/run-in-tmp";
 import { runWrangler } from "./helpers/run-wrangler";
 import writeWranglerToml from "./helpers/write-wrangler-toml";
-import type { Config } from "../config";
 import type { WorkerMetadata } from "../create-worker-upload-form";
 import type { KVNamespaceInfo } from "../kv";
 import type { FormData, File } from "undici";
@@ -465,9 +464,8 @@ export default{
 
     it('should error if a site definition doesn\'t have a "bucket" field', async () => {
       writeWranglerToml({
-        site: {
-          "entry-point": "./index.js",
-        } as Config["site"],
+        // @ts-expect-error we're intentionally setting an invalid config
+        site: {},
       });
       writeWorkerSource();
       mockUploadWorkerRequest();
@@ -489,7 +487,7 @@ export default{
       expect(std.warn).toMatchInlineSnapshot(`""`);
     });
 
-    it("should warn if there is a `site.entry-point` configuration", async () => {
+    it("should error if there is a `site.entry-point` configuration", async () => {
       const assets = [
         { filePath: "assets/file-1.txt", content: "Content of file-1" },
         { filePath: "assets/file-2.txt", content: "Content of file-2" },
@@ -513,22 +511,26 @@ export default{
       mockKeyListRequest(kvNamespace.id, []);
       mockUploadAssetsToKVRequest(kvNamespace.id, assets);
 
-      await runWrangler("publish ./index.js");
+      await expect(runWrangler("publish ./index.js")).rejects
+        .toThrowErrorMatchingInlineSnapshot(`
+              "Processing wrangler.toml configuration:
+                - DEPRECATION: \\"site.entry-point\\":
+                  The \`site.entry-point\` config field is no longer used.
+                  The entry-point should be specified via the command line or the \`main\` config field."
+            `);
 
-      expect(std.out).toMatchInlineSnapshot(`
-        "reading assets/file-1.txt...
-        uploading as assets/file-1.2ca234f380.txt...
-        reading assets/file-2.txt...
-        uploading as assets/file-2.5938485188.txt...
-        Uploaded test-name (TIMINGS)
-        Published test-name (TIMINGS)
-          test-name.test-sub-domain.workers.dev"
+      expect(std.out).toMatchInlineSnapshot(`""`);
+      expect(std.err).toMatchInlineSnapshot(`
+        "Processing wrangler.toml configuration:
+          - DEPRECATION: \\"site.entry-point\\":
+            The \`site.entry-point\` config field is no longer used.
+            The entry-point should be specified via the command line or the \`main\` config field.
+
+        [32m%s[0m If you think this is a bug then please create an issue at https://github.com/cloudflare/wrangler2/issues/new."
       `);
-      expect(std.err).toMatchInlineSnapshot(`""`);
       expect(std.warn).toMatchInlineSnapshot(`
-        "Deprecation notice: The \`site.entry-point\` config field is no longer used.
-        The entry-point should be specified via the command line (e.g. \`wrangler publish path/to/script\`) or the \`main\` config field.
-        Please remove the \`site.entry-point\` field from the \`wrangler.toml\` file."
+        "Processing wrangler.toml configuration:
+          - Unexpected fields found in site field: \\"entry-point\\""
       `);
     });
 

diff --git a/packages/wrangler/src/config/config.ts b/packages/wrangler/src/config/config.ts
@@ -255,7 +255,6 @@ export interface DeprecatedUpload {
    * Defaults to the directory containing the wrangler.toml file.
    *
    * @deprecated
-   * @breaking In wrangler 1, this defaults to ./dist, whereas in wrangler 2 it defaults to ./
    */
   dir?: string;
 

diff --git a/packages/wrangler/src/config/validation-helpers.ts b/packages/wrangler/src/config/validation-helpers.ts
@@ -5,19 +5,22 @@ import type { Environment, RawEnvironment } from "./environment";
 /**
  *  Mark a field as deprecated.
  *
- * This function will add a diagnostics warning if the deprecated field is found in the `rawEnv`.
+ * This function will add a diagnostics warning if the deprecated field is found in the `rawEnv` (or an error if it's also a breaking deprecation)
  * The `fieldPath` is a dot separated property path, e.g. `"build.upload.format"`.
  */
 export function deprecated<T extends object>(
   diagnostics: Diagnostics,
   config: T,
   fieldPath: DeepKeyOf<T>,
   message: string,
-  remove: boolean
+  remove: boolean,
+  breaking = false
 ): void {
   const result = unwindPropertyPath(config, fieldPath);
   if (result !== undefined && result.field in result.container) {
-    diagnostics.warnings.push(`DEPRECATION: "${fieldPath}":\n${message}`);
+    (breaking ? diagnostics.errors : diagnostics.warnings).push(
+      `DEPRECATION: "${fieldPath}":\n${message}`
+    );
     if (remove) {
       delete (result.container as Record<string, unknown>)[result.field];
     }

diff --git a/packages/wrangler/src/config/validation.ts b/packages/wrangler/src/config/validation.ts
@@ -61,6 +61,15 @@ export function normalizeAndValidateConfig(
     true
   );
 
+  deprecated(
+    diagnostics,
+    rawConfig,
+    `site.entry-point`,
+    `The \`site.entry-point\` config field is no longer used.\nThe entry-point should be specified via the command line or the \`main\` config field.`,
+    false,
+    true
+  );
+
   const { deprecatedUpload, ...build } = normalizeAndValidateBuild(
     diagnostics,
     rawConfig,
@@ -342,25 +351,12 @@ function normalizeAndValidateSite(
   rawSite: Config["site"]
 ): Config["site"] {
   if (rawSite !== undefined) {
-    const {
-      bucket,
-      "entry-point": entryPoint,
-      include = [],
-      exclude = [],
-      ...rest
-    } = rawSite;
+    const { bucket, include = [], exclude = [], ...rest } = rawSite;
     validateAdditionalProperties(diagnostics, "site", Object.keys(rest), []);
     validateRequiredProperty(diagnostics, "site", "bucket", bucket, "string");
-    validateOptionalProperty(
-      diagnostics,
-      "site",
-      "entry_point",
-      entryPoint,
-      "string"
-    );
     validateTypedArray(diagnostics, "sites.include", include, "string");
     validateTypedArray(diagnostics, "sites.exclude", exclude, "string");
-    return { bucket, "entry-point": entryPoint, include, exclude };
+    return { bucket, include, exclude };
   }
   return undefined;
 }

diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx
@@ -788,14 +788,6 @@ export async function main(argv: string[]): Promise<void> {
         );
       }
 
-      if (config.site?.["entry-point"]) {
-        console.warn(
-          "Deprecation notice: The `site.entry-point` config field is no longer used.\n" +
-            "The entry-point is specified via the command line (e.g. `wrangler dev path/to/script`).\n" +
-            "Please remove the `site.entry-point` field from the `wrangler.toml` file."
-        );
-      }
-
       const accountId = !args.local ? await requireAuth(config) : undefined;
 
       // TODO: if worker_dev = false and no routes, then error (only for dev)

diff --git a/packages/wrangler/src/publish.ts b/packages/wrangler/src/publish.ts
@@ -64,14 +64,6 @@ export default async function publish(props: Props): Promise<void> {
     'You need to provide a name when publishing a worker. Either pass it as a cli arg with `--name <name>` or in your config file as `name = "<name>"`'
   );
 
-  if (config.site?.["entry-point"]) {
-    console.warn(
-      "Deprecation notice: The `site.entry-point` config field is no longer used.\n" +
-        "The entry-point should be specified via the command line (e.g. `wrangler publish path/to/script`) or the `main` config field.\n" +
-        "Please remove the `site.entry-point` field from the `wrangler.toml` file."
-    );
-  }
-
   assert(
     !config.site || config.site.bucket,
     "A [site] definition requires a `bucket` field with a path to the site's public directory."