refactor: remove process.exit() from the pages code (#747)

This enables simpler testing, as we do not have to spawn new child processes to avoid the `process.exit()` from killing the jest process. As part of the refactor, some of the `Error` classes have been moved to a shared `errors.ts` file.
cloudflare · Apr 2, 2022 · db6b830 · db6b830
1 parent 3e25dcb
commit db6b830
Show file tree

Hide file tree

Showing 9 changed files with 85 additions and 75 deletions.
diff --git a/.changeset/polite-lemons-shop.md b/.changeset/polite-lemons-shop.md
@@ -0,0 +1,11 @@
+---
+"wrangler": patch
+---
+
+refactor: remove `process.exit()` from the pages code
+
+This enables simpler testing, as we do not have to spawn new child processes
+to avoid the `process.exit()` from killing the jest process.
+
+As part of the refactor, some of the `Error` classes have been moved to a
+shared `errors.ts` file.
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -17,6 +17,7 @@
     "pgrep",
     "PKCE",
     "Positionals",
+    "scandir",
     "selfsigned",
     "textfile",
     "tsbuildinfo",
@@ -28,7 +29,12 @@
     "websockets",
     "xxhash"
   ],
-  "cSpell.ignoreWords": ["TESTTEXTBLOBNAME", "TESTWASMNAME", "yxxx"],
+  "cSpell.ignoreWords": [
+    "TESTTEXTBLOBNAME",
+    "TESTWASMNAME",
+    "extensionless",
+    "yxxx"
+  ],
   "eslint.runtime": "node",
   "files.trimTrailingWhitespace": true
 }
diff --git a/packages/wrangler/src/__tests__/index.test.ts b/packages/wrangler/src/__tests__/index.test.ts
@@ -845,14 +845,14 @@ describe("wrangler", () => {
     it("should throw an error if the deprecated command is used with positional arguments", async () => {
       await expect(runWrangler("preview GET")).rejects
         .toThrowErrorMatchingInlineSnapshot(`
-              "DEPRECATION WARNING:
+              "DEPRECATION:
               The \`wrangler preview\` command has been deprecated.
               Try using \`wrangler dev\` to to try out a worker during development.
               "
             `);
       await expect(runWrangler(`preview GET "SomeBody"`)).rejects
         .toThrowErrorMatchingInlineSnapshot(`
-              "DEPRECATION WARNING:
+              "DEPRECATION:
               The \`wrangler preview\` command has been deprecated.
               Try using \`wrangler dev\` to to try out a worker during development.
               "
@@ -970,15 +970,15 @@ describe("wrangler", () => {
     it("should print a deprecation message for 'generate'", async () => {
       await runWrangler("generate").catch((err) => {
         expect(err.message).toMatchInlineSnapshot(`
-          "DEPRECATION WARNING:
+          "DEPRECATION:
           \`wrangler generate\` has been deprecated, please refer to https://github.com/cloudflare/wrangler2/blob/main/docs/deprecations.md#generate for alternatives"
         `);
       });
     });
     it("should print a deprecation message for 'build'", async () => {
       await runWrangler("build").catch((err) => {
         expect(err.message).toMatchInlineSnapshot(`
-          "DEPRECATION WARNING:
+          "DEPRECATION:
           \`wrangler build\` has been deprecated, please refer to https://github.com/cloudflare/wrangler2/blob/main/docs/deprecations.md#build for alternatives"
         `);
       });

diff --git a/packages/wrangler/src/__tests__/pages.test.ts b/packages/wrangler/src/__tests__/pages.test.ts
@@ -1,25 +1,9 @@
-import { execaSync } from "execa";
-import { getPackageManager } from "../package-manager";
 import { mockConsoleMethods } from "./helpers/mock-console";
 import { runInTempDir } from "./helpers/run-in-tmp";
 import { runWrangler } from "./helpers/run-wrangler";
-import type { PackageManager } from "../package-manager";
 
-describe("subcommand implicit help ran on imcomplete command execution", () => {
-  let mockPackageManager: PackageManager;
+describe("subcommand implicit help ran on incomplete command execution", () => {
   runInTempDir();
-
-  beforeEach(() => {
-    mockPackageManager = {
-      cwd: process.cwd(),
-      // @ts-expect-error we're making a fake package manager here
-      type: "mockpm",
-      addDevDeps: jest.fn(),
-      install: jest.fn(),
-    };
-    (getPackageManager as jest.Mock).mockResolvedValue(mockPackageManager);
-  });
-
   const std = mockConsoleMethods();
   function endEventLoop() {
     return new Promise((resolve) => setImmediate(resolve));
@@ -46,36 +30,28 @@ describe("subcommand implicit help ran on imcomplete command execution", () => {
       🚧 'wrangler pages <command>' is a beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose"
     `);
   });
-});
 
-describe("beta message for subcommands", () => {
-  const betaMsg =
-    "🚧 'wrangler pages <command>' is a beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose";
-  const isWindows = process.platform === "win32";
-  it("should display for pages:dev", async () => {
-    let err: Error | undefined;
-    try {
-      execaSync("npx", ["wrangler", "pages", "dev"], {
-        shell: isWindows,
-        env: { BROWSER: "none", ...process.env },
-      }).stderr;
-    } catch (e: unknown) {
-      err = e as Error;
-    }
-    expect(err?.message.includes(betaMsg)).toBe(true);
-  });
+  describe("beta message for subcommands", () => {
+    it("should display for pages:dev", async () => {
+      await expect(
+        runWrangler("pages dev")
+      ).rejects.toThrowErrorMatchingInlineSnapshot(
+        `"Must specify a directory of static assets to serve or a command to run."`
+      );
+
+      expect(std.out).toMatchInlineSnapshot(
+        `"🚧 'wrangler pages <command>' is a beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose"`
+      );
+    });
+
+    // Note that `wrangler pages functions` does nothing...
 
-  it("should display for pages:functions", async () => {
-    let err: Error | undefined;
-    try {
-      execaSync("npx", ["wrangler", "pages", "functions", "build"], {
-        shell: isWindows,
-        env: { BROWSER: "none", ...process.env },
-      });
-    } catch (e: unknown) {
-      err = e as Error;
-    }
+    it("should display for pages:functions:build", async () => {
+      await expect(runWrangler("pages functions build")).rejects.toThrowError();
 
-    expect(err?.message.includes(betaMsg)).toBe(true);
+      expect(std.out).toMatchInlineSnapshot(
+        `"🚧 'wrangler pages <command>' is a beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose"`
+      );
+    });
   });
 });
diff --git a/packages/wrangler/src/__tests__/route.test.ts b/packages/wrangler/src/__tests__/route.test.ts
@@ -9,7 +9,7 @@ describe("wrangler route", () => {
   it("shows a deprecation notice when `wrangler route` is run", async () => {
     await expect(runWrangler("route")).rejects
       .toThrowErrorMatchingInlineSnapshot(`
-            "DEPRECATION WARNING:
+            "DEPRECATION:
             \`wrangler route\` has been deprecated.
             Please use wrangler.toml and/or \`wrangler publish --routes\` to modify routes"
           `);
@@ -18,7 +18,7 @@ describe("wrangler route", () => {
   it("shows a deprecation notice when `wrangler route delete` is run", async () => {
     await expect(runWrangler("route delete")).rejects
       .toThrowErrorMatchingInlineSnapshot(`
-            "DEPRECATION WARNING:
+            "DEPRECATION:
             \`wrangler route delete\` has been deprecated.
             Remove the unwanted route(s) from wrangler.toml and run \`wrangler publish\` to remove your worker from those routes."
           `);
@@ -27,7 +27,7 @@ describe("wrangler route", () => {
   it("shows a deprecation notice when `wrangler route delete <id>` is run", async () => {
     await expect(runWrangler("route delete some-zone-id")).rejects
       .toThrowErrorMatchingInlineSnapshot(`
-            "DEPRECATION WARNING:
+            "DEPRECATION:
             \`wrangler route delete\` has been deprecated.
             Remove the unwanted route(s) from wrangler.toml and run \`wrangler publish\` to remove your worker from those routes."
           `);
@@ -36,7 +36,7 @@ describe("wrangler route", () => {
   it("shows a deprecation notice when `wrangler route list` is run", async () => {
     await expect(runWrangler("route list")).rejects
       .toThrowErrorMatchingInlineSnapshot(`
-            "DEPRECATION WARNING:
+            "DEPRECATION:
             \`wrangler route list\` has been deprecated.
             Refer to wrangler.toml for a list of routes the worker will be deployed to upon publishing.
             Refer to the Cloudflare Dashboard to see the routes this worker is currently running on."

diff --git a/packages/wrangler/src/cli.ts b/packages/wrangler/src/cli.ts
@@ -1,5 +1,6 @@
 import process from "process";
 import { hideBin } from "yargs/helpers";
+import { FatalError } from "./errors";
 import { reportError, initReporting } from "./reporting";
 import { main } from ".";
 
@@ -11,9 +12,10 @@ process.on("uncaughtExceptionMonitor", async (err, origin) => {
   await reportError(err, origin);
 });
 
-main(hideBin(process.argv)).catch(() => {
+main(hideBin(process.argv)).catch((e) => {
   // The logging of any error that was thrown from `main()` is handled in the `yargs.fail()` handler.
   // Here we just want to ensure that the process exits with a non-zero code.
   // We don't want to do this inside the `main()` function, since that would kill the process when running our tests.
-  process.exit(1);
+  const exitCode = (e instanceof FatalError && e.code) || 1;
+  process.exit(exitCode);
 });
diff --git a/packages/wrangler/src/errors.ts b/packages/wrangler/src/errors.ts
@@ -0,0 +1,11 @@
+export class DeprecationError extends Error {
+  constructor(message: string) {
+    super(`DEPRECATION:\n${message}`);
+  }
+}
+
+export class FatalError extends Error {
+  constructor(message?: string, readonly code?: number) {
+    super(message);
+  }
+}
diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx
@@ -18,6 +18,7 @@ import { createWorkerUploadForm } from "./create-worker-upload-form";
 import Dev from "./dev/dev";
 import { confirm, prompt } from "./dialogs";
 import { getEntry } from "./entry";
+import { DeprecationError } from "./errors";
 import {
   getNamespaceId,
   listNamespaces,
@@ -211,11 +212,6 @@ function demandOneOfOption(...options: string[]) {
 }
 
 class CommandLineArgsError extends Error {}
-class DeprecationError extends Error {
-  constructor(message: string) {
-    super(`DEPRECATION WARNING:\n${message}`);
-  }
-}
 
 export async function main(argv: string[]): Promise<void> {
   const wrangler = makeCLI(argv)

diff --git a/packages/wrangler/src/pages.tsx b/packages/wrangler/src/pages.tsx
@@ -10,6 +10,7 @@ import { getType } from "mime";
 import { buildWorker } from "../pages/functions/buildWorker";
 import { generateConfigFromFileTree } from "../pages/functions/filepath-routing";
 import { writeRoutesModule } from "../pages/functions/routes";
+import { FatalError } from "./errors";
 import openInBrowser from "./open-in-browser";
 import { toUrlPath } from "./paths";
 import type { Config } from "../pages/functions/routes";
@@ -25,17 +26,20 @@ import type { BuilderCallback } from "yargs";
 export const pagesBetaWarning =
   "🚧 'wrangler pages <command>' is a beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose";
 
-const EXIT_CALLBACKS: (() => void)[] = [];
-const EXIT = (message?: string, code?: number) => {
-  if (message) console.log(message);
-  if (code) process.exitCode = code;
-  EXIT_CALLBACKS.forEach((callback) => callback());
+const CLEANUP_CALLBACKS: (() => void)[] = [];
+const CLEANUP = () => {
+  CLEANUP_CALLBACKS.forEach((callback) => callback());
   RUNNING_BUILDERS.forEach((builder) => builder.stop?.());
-  process.exit(code);
 };
 
-process.on("SIGINT", () => EXIT());
-process.on("SIGTERM", () => EXIT());
+process.on("SIGINT", () => {
+  CLEANUP();
+  process.exit();
+});
+process.on("SIGTERM", () => {
+  CLEANUP();
+  process.exit();
+});
 
 function isWindows() {
   return process.platform === "win32";
@@ -108,11 +112,13 @@ async function spawnProxyProcess({
   port?: number;
   command: (string | number)[];
 }): Promise<void | number> {
-  if (command.length === 0)
-    return EXIT(
+  if (command.length === 0) {
+    CLEANUP();
+    throw new FatalError(
       "Must specify a directory of static assets to serve or a command to run.",
       1
     );
+  }
 
   console.log(`Running ${command.join(" ")}...`);
   const proxy = spawn(
@@ -126,7 +132,7 @@ async function spawnProxyProcess({
       },
     }
   );
-  EXIT_CALLBACKS.push(() => {
+  CLEANUP_CALLBACKS.push(() => {
     proxy.kill();
   });
 
@@ -157,7 +163,8 @@ async function spawnProxyProcess({
       .filter((port) => port !== undefined)[0];
 
     if (port === undefined) {
-      return EXIT(
+      CLEANUP();
+      throw new FatalError(
         "Could not automatically determine proxy port. Please specify the proxy port with --proxy.",
         1
       );
@@ -970,13 +977,14 @@ export const pages: BuilderCallback<unknown, unknown> = (yargs) => {
             });
           }
 
-          EXIT_CALLBACKS.push(() => {
+          CLEANUP_CALLBACKS.push(() => {
             server.close();
             miniflare.dispose().catch((err) => miniflare.log.error(err));
           });
         } catch (e) {
           miniflare.log.error(e as Error);
-          EXIT("Could not start Miniflare.", 1);
+          CLEANUP();
+          throw new FatalError("Could not start Miniflare.", 1);
         }
       }
     )