Refactor pages code to pass strict-null checks

Author: petebacondarwin

packages/wrangler/pages/functions/filepath-routing.ts

@@ -52,7 +52,7 @@ export async function generateConfigFromFileTree({
             const declaration = node.declaration;
 
             // `export async function onRequest() {...}`
-            if (declaration.type === "FunctionDeclaration") {
+            if (declaration.type === "FunctionDeclaration" && declaration.id) {
               exportNames.push(declaration.id.name);
             }
 
@@ -155,12 +155,10 @@ export async function generateConfigFromFileTree({
 // more specific routes aren't occluded from matching due to
 // less specific routes appearing first in the route list.
 export function compareRoutes(a: string, b: string) {
-  function parseRoutePath(routePath: string) {
-    let [method, segmentedPath] = routePath.split(" ");
-    if (!segmentedPath) {
-      segmentedPath = method;
-      method = null;
-    }
+  function parseRoutePath(routePath: string): [string | null, string[]] {
+    const parts = routePath.split(" ", 2);
+    const segmentedPath = parts.pop() ?? "";
+    const method = parts.pop() ?? null;
 
     const segments = segmentedPath.slice(1).split("/").filter(Boolean);
     return [method, segments];
@@ -204,7 +202,7 @@ async function forEachFile<T>(
   const searchPaths = [baseDir];
   const returnValues: T[] = [];
 
-  while (searchPaths.length) {
+  while (isNotEmpty(searchPaths)) {
     const cwd = searchPaths.shift();
     const dir = await fs.readdir(cwd, { withFileTypes: true });
     for (const entry of dir) {
@@ -219,3 +217,10 @@ async function forEachFile<T>(
 
   return returnValues;
 }
+
+interface NonEmptyArray<T> extends Array<T> {
+  shift(): T;
+}
+function isNotEmpty<T>(array: T[]): array is NonEmptyArray<T> {
+  return array.length > 0;
+}

packages/wrangler/pages/functions/routes.ts

@@ -114,7 +114,7 @@ export function parseConfig(config: Config, baseDir: string) {
     });
   }
 
-  for (const [route, props] of Object.entries(config.routes)) {
+  for (const [route, props] of Object.entries(config.routes ?? {})) {
     let [_methods, routePath] = route.split(" ");
     if (!routePath) {
       routePath = _methods;

packages/wrangler/pages/functions/template-worker.ts

@@ -33,17 +33,16 @@ declare const routes: RouteHandler[];
 declare const __FALLBACK_SERVICE__: string;
 
 // expect an ASSETS fetcher binding pointing to the asset-server stage
-type Env = {
-  [name: string]: unknown;
-  ASSETS: { fetch(url: string, init: RequestInit): Promise<Response> };
+type FetchEnv = {
+  [name: string]: { fetch: typeof fetch };
+  ASSETS: { fetch: typeof fetch };
 };
 
 type WorkerContext = {
   waitUntil: (promise: Promise<unknown>) => void;
 };
 
-// eslint-disable-next-line @typescript-eslint/no-unused-vars -- `env` can be used by __FALLBACK_SERVICE_FETCH__
-function* executeRequest(request: Request, env: Env) {
+function* executeRequest(request: Request, _env: FetchEnv) {
   const requestPath = new URL(request.url).pathname;
 
   // First, iterate through the routes (backwards) and execute "middlewares" on partial route matches
@@ -88,35 +87,22 @@ function* executeRequest(request: Request, env: Env) {
       break;
     }
   }
-
-  // Finally, yield to the fallback service (`env.ASSETS.fetch` in Pages' case)
-  return {
-    handler: () =>
-      __FALLBACK_SERVICE__
-        ? // @ts-expect-error expecting __FALLBACK_SERVICE__ to be the name of a service binding, so fetch should be defined
-          env[__FALLBACK_SERVICE__].fetch(request)
-        : fetch(request),
-    params: {} as Params,
-  };
 }
 
 export default {
-  async fetch(request: Request, env: Env, workerContext: WorkerContext) {
+  async fetch(request: Request, env: FetchEnv, workerContext: WorkerContext) {
     const handlerIterator = executeRequest(request, env);
     const data = {}; // arbitrary data the user can set between functions
     const next = async (input?: RequestInfo, init?: RequestInit) => {
       if (input !== undefined) {
         request = new Request(input, init);
       }
 
-      const { value } = handlerIterator.next();
-      if (value) {
-        const { handler, params } = value;
-        const context: EventContext<
-          unknown,
-          string,
-          Record<string, unknown>
-        > = {
+      const result = handlerIterator.next();
+      // Note we can't use `!result.done` because this doesn't narrow to the correct type
+      if (result.done == false) {
+        const { handler, params } = result.value;
+        const context = {
           request: new Request(request.clone()),
           next,
           params,
@@ -132,6 +118,12 @@ export default {
           [101, 204, 205, 304].includes(response.status) ? null : response.body,
           response
         );
+      } else if (__FALLBACK_SERVICE__) {
+        // There are no more handlers so finish with the fallback service (`env.ASSETS.fetch` in Pages' case)
+        return env[__FALLBACK_SERVICE__].fetch(request);
+      } else {
+        // There was not fallback service so actually make the request to the origin.
+        return fetch(request);
       }
     };
 

packages/wrangler/src/pages.tsx

@@ -1,6 +1,5 @@
 /* eslint-disable no-shadow */
 
-import assert from "assert";
 import type { BuilderCallback } from "yargs";
 import { join } from "path";
 import { tmpdir } from "os";
@@ -22,7 +21,7 @@ import { generateConfigFromFileTree } from "../pages/functions/filepath-routing"
 import type { Headers, Request, fetch } from "@miniflare/core";
 import type { MiniflareOptions } from "miniflare";
 
-const EXIT_CALLBACKS = [];
+const EXIT_CALLBACKS: (() => void)[] = [];
 const EXIT = (message?: string, code?: number) => {
   if (message) console.log(message);
   if (code) process.exitCode = code;
@@ -122,7 +121,9 @@ async function spawnProxyProcess({
       },
     }
   );
-  EXIT_CALLBACKS.push(() => proxy.kill());
+  EXIT_CALLBACKS.push(() => {
+    proxy.kill();
+  });
 
   proxy.stdout.on("data", (data) => {
     console.log(`[proxy]: ${data}`);
@@ -862,11 +863,13 @@ export const pages: BuilderCallback<unknown, unknown> = (yargs) => {
         // internally just waits for that promise to resolve.
         await scriptReadyPromise;
 
-        // Should only be called if no proxyPort, using `assert.fail()` here
-        // means the type of `assetsFetch` is still `typeof fetch`
-        const assetsFetch = proxyPort
-          ? () => assert.fail()
-          : await generateAssetsFetch(directory);
+        // `assetsFetch()` will only be called if there is `proxyPort` defined.
+        // We only define `proxyPort`, above, when there is no `directory` defined.
+        const assetsFetch =
+          directory !== undefined
+            ? await generateAssetsFetch(directory)
+            : invalidAssetsFetch;
+
         const miniflare = new Miniflare({
           port,
           watch: true,
@@ -1029,3 +1032,9 @@ export const pages: BuilderCallback<unknown, unknown> = (yargs) => {
       )
     );
 };
+
+const invalidAssetsFetch: typeof fetch = () => {
+  throw new Error(
+    "Trying to fetch assets directly when there is no `directory` option specified, and not in `local` mode."
+  );
+};