refactor: clean up pages routing

cloudflare · Feb 4, 2022 · e355f25 · e355f25
1 parent 841006f
commit e355f25
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 91 deletions.
diff --git a/.changeset/itchy-fans-smoke.md b/.changeset/itchy-fans-smoke.md
@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+refactor: clean up pages routing
diff --git a/packages/wrangler/pages/functions/filepath-routing.test.ts b/packages/wrangler/pages/functions/filepath-routing.test.ts
@@ -1,47 +1,66 @@
 import { toUrlPath } from "../../src/paths";
 import { compareRoutes } from "./filepath-routing";
+import type { HTTPMethod, RouteConfig } from "./routes";
 
 describe("compareRoutes()", () => {
-  const url = toUrlPath;
   test("routes / last", () => {
-    expect(compareRoutes([url("/")], [url("/foo")])).toBeGreaterThanOrEqual(1);
-    expect(compareRoutes([url("/")], [url("/:foo")])).toBeGreaterThanOrEqual(1);
-    expect(compareRoutes([url("/")], [url("/:foo*")])).toBeGreaterThanOrEqual(
-      1
-    );
+    expect(
+      compareRoutes(routeConfig("/"), routeConfig("/foo"))
+    ).toBeGreaterThanOrEqual(1);
+    expect(
+      compareRoutes(routeConfig("/"), routeConfig("/:foo"))
+    ).toBeGreaterThanOrEqual(1);
+    expect(
+      compareRoutes(routeConfig("/"), routeConfig("/:foo*"))
+    ).toBeGreaterThanOrEqual(1);
   });
 
   test("routes with fewer segments come after those with more segments", () => {
     expect(
-      compareRoutes([url("/foo")], [url("/foo/bar")])
+      compareRoutes(routeConfig("/foo"), routeConfig("/foo/bar"))
     ).toBeGreaterThanOrEqual(1);
     expect(
-      compareRoutes([url("/foo")], [url("/foo/bar/cat")])
+      compareRoutes(routeConfig("/foo"), routeConfig("/foo/bar/cat"))
     ).toBeGreaterThanOrEqual(1);
   });
 
   test("routes with wildcard segments come after those without", () => {
-    expect(compareRoutes([url("/:foo*")], [url("/foo")])).toBe(1);
-    expect(compareRoutes([url("/:foo*")], [url("/:foo")])).toBe(1);
+    expect(compareRoutes(routeConfig("/:foo*"), routeConfig("/foo"))).toBe(1);
+    expect(compareRoutes(routeConfig("/:foo*"), routeConfig("/:foo"))).toBe(1);
   });
 
   test("routes with dynamic segments come after those without", () => {
-    expect(compareRoutes([url("/:foo")], [url("/foo")])).toBe(1);
+    expect(compareRoutes(routeConfig("/:foo"), routeConfig("/foo"))).toBe(1);
   });
 
   test("routes with dynamic segments occurring earlier come after those with dynamic segments in later positions", () => {
-    expect(compareRoutes([url("/foo/:id/bar")], [url("/foo/bar/:id")])).toBe(1);
+    expect(
+      compareRoutes(routeConfig("/foo/:id/bar"), routeConfig("/foo/bar/:id"))
+    ).toBe(1);
   });
 
   test("routes with no HTTP method come after those specifying a method", () => {
-    expect(compareRoutes([url("/foo")], [url("/foo"), "GET"])).toBe(1);
+    expect(compareRoutes(routeConfig("/foo"), routeConfig("/foo", "GET"))).toBe(
+      1
+    );
   });
 
   test("two equal routes are sorted according to their original position in the list", () => {
-    expect(compareRoutes([url("/foo"), "GET"], [url("/foo"), "GET"])).toBe(0);
+    expect(
+      compareRoutes(routeConfig("/foo", "GET"), routeConfig("/foo", "GET"))
+    ).toBe(0);
   });
 
   test("it returns -1 if the first argument should appear first in the list", () => {
-    expect(compareRoutes([url("/foo"), "GET"], [url("/foo")])).toBe(-1);
+    expect(compareRoutes(routeConfig("/foo", "GET"), routeConfig("/foo"))).toBe(
+      -1
+    );
   });
 });
+
+function routeConfig(routePath: string, method?: string): RouteConfig {
+  return {
+    routePath: toUrlPath(routePath),
+    method: method as HTTPMethod,
+  };
+}
diff --git a/packages/wrangler/pages/functions/filepath-routing.ts b/packages/wrangler/pages/functions/filepath-routing.ts
@@ -5,22 +5,17 @@ import * as acornWalk from "acorn-walk";
 import { transform } from "esbuild";
 import { toUrlPath } from "../../src/paths";
 import type { UrlPath } from "../../src/paths";
-import type { Config, RouteConfig } from "./routes";
+import type { HTTPMethod, RouteConfig } from "./routes";
 import type { ExportNamedDeclaration, Identifier } from "estree";
 
-type Arguments = {
-  baseDir: string;
-  baseURL: UrlPath;
-};
-
-type Method = "GET" | "POST" | "PUT" | "PATCH" | "DELETE" | "OPTIONS" | "HEAD";
-type RouteKey = [UrlPath, Method] | [UrlPath, undefined] | [UrlPath];
-
 export async function generateConfigFromFileTree({
   baseDir,
   baseURL,
-}: Arguments) {
-  let routeEntries: [RouteKey, RouteConfig][] = [];
+}: {
+  baseDir: string;
+  baseURL: UrlPath;
+}) {
+  let routeEntries: RouteConfig[] = [];
 
   if (!baseURL.startsWith("/")) {
     baseURL = `/${baseURL}` as UrlPath;
@@ -83,10 +78,9 @@ export async function generateConfigFromFileTree({
           }
 
           for (const exportName of exportNames) {
-            const [match, method] =
-              exportName.match(
-                /^onRequest(Get|Post|Put|Patch|Delete|Options|Head)?$/
-              ) ?? [];
+            const [match, method] = (exportName.match(
+              /^onRequest(Get|Post|Put|Patch|Delete|Options|Head)?$/
+            ) ?? []) as (string | undefined)[];
 
             if (match) {
               const basename = path.basename(filepath).slice(0, -ext.length);
@@ -113,20 +107,15 @@ export async function generateConfigFromFileTree({
               routePath = routePath.replace(/\[\[(.+)]]/g, ":$1*"); // transform [[id]] => :id*
               routePath = routePath.replace(/\[(.+)]/g, ":$1"); // transform [id] => :id
 
-              const routeUrlPath = toUrlPath(routePath);
-              const routeKey: RouteKey = [
-                routeUrlPath,
-                method ? (method.toUpperCase() as Method) : undefined,
-              ];
-
-              routeEntries.push([
-                routeKey,
-                {
-                  [isMiddlewareFile ? "middleware" : "module"]: [
-                    `${path.relative(baseDir, filepath)}:${exportName}`,
-                  ],
-                },
-              ]);
+              const routeEntry: RouteConfig = {
+                routePath: toUrlPath(routePath),
+                method: method?.toUpperCase() as HTTPMethod,
+                [isMiddlewareFile ? "middleware" : "module"]: [
+                  `${path.relative(baseDir, filepath)}:${exportName}`,
+                ],
+              };
+
+              routeEntries.push(routeEntry);
             }
           }
         },
@@ -136,41 +125,33 @@ export async function generateConfigFromFileTree({
 
   // Combine together any routes (index routes) which contain both a module and a middleware
   routeEntries = routeEntries.reduce(
-    (acc: typeof routeEntries, [routePath, routeHandler]) => {
+    (acc: typeof routeEntries, { routePath, ...rest }) => {
       const existingRouteEntry = acc.find(
-        (routeEntry) => routeEntry[0] === routePath
+        (routeEntry) => routeEntry.routePath === routePath
       );
       if (existingRouteEntry !== undefined) {
-        existingRouteEntry[1] = {
-          ...existingRouteEntry[1],
-          ...routeHandler,
-        };
+        Object.assign(existingRouteEntry, rest);
       } else {
-        acc.push([routePath, routeHandler]);
+        acc.push({ routePath, ...rest });
       }
       return acc;
     },
     []
   );
 
-  routeEntries.sort(([a], [b]) => compareRoutes(a, b));
+  routeEntries.sort((a, b) => compareRoutes(a, b));
 
   return {
-    routes: Object.fromEntries(
-      routeEntries.map(([routeKey, routeConfig]) => [
-        stringifyRouteKey(routeKey),
-        routeConfig,
-      ])
-    ),
-  } as Config;
+    routes: routeEntries,
+  };
 }
 
 // Ensure routes are produced in order of precedence so that
 // more specific routes aren't occluded from matching due to
 // less specific routes appearing first in the route list.
 export function compareRoutes(
-  [routePathA, methodA]: RouteKey,
-  [routePathB, methodB]: RouteKey
+  { routePath: routePathA, method: methodA }: RouteConfig,
+  { routePath: routePathB, method: methodB }: RouteConfig
 ) {
   function parseRoutePath(routePath: UrlPath): string[] {
     return routePath.slice(1).split("/").filter(Boolean);
@@ -236,7 +217,3 @@ interface NonEmptyArray<T> extends Array<T> {
 function isNotEmpty<T>(array: T[]): array is NonEmptyArray<T> {
   return array.length > 0;
 }
-
-function stringifyRouteKey(routeKey: RouteKey): string {
-  return (routeKey[1] ? `${routeKey[1]} ` : "") + routeKey[0];
-}
diff --git a/packages/wrangler/pages/functions/routes.ts b/packages/wrangler/pages/functions/routes.ts
@@ -1,6 +1,5 @@
 import fs from "node:fs/promises";
 import path from "node:path";
-import { toUrlPath } from "../../src/paths";
 import { isValidIdentifier, normalizeIdentifier } from "./identifiers";
 import type { UrlPath } from "../../src/paths";
 
@@ -22,21 +21,19 @@ export function isHTTPMethod(
 
 export type RoutesCollection = Array<{
   routePath: UrlPath;
-  methods: HTTPMethod[];
+  method?: HTTPMethod;
   modules: string[];
   middlewares: string[];
 }>;
 
 export type Config = {
-  routes?: RoutesConfig;
+  routes?: RouteConfig[];
   schedules?: unknown;
 };
 
-export type RoutesConfig = {
-  [route: UrlPath]: RouteConfig;
-};
-
 export type RouteConfig = {
+  routePath: UrlPath;
+  method?: HTTPMethod;
   middleware?: string | string[];
   module?: string | string[];
 };
@@ -116,16 +113,10 @@ export function parseConfig(config: Config, baseDir: string) {
     });
   }
 
-  for (const [route, props] of Object.entries(config.routes ?? {})) {
-    let [_methods, routePath] = route.split(" ");
-    if (!routePath) {
-      routePath = _methods;
-      _methods = "";
-    }
-
+  for (const { routePath, method, ...props } of config.routes ?? []) {
     routes.push({
-      routePath: toUrlPath(routePath),
-      methods: _methods.split("|").filter(isHTTPMethod),
+      routePath,
+      method,
       middlewares: parseModuleIdentifiers(props.middleware),
       modules: parseModuleIdentifiers(props.module),
     });
@@ -150,7 +141,7 @@ export const routes = [
     .map(
       (route) => `  {
       routePath: "${route.routePath}",
-      methods: ${JSON.stringify(route.methods)},
+      method: "${route.method}",
       middlewares: [${route.middlewares.join(", ")}],
       modules: [${route.modules.join(", ")}],
     },`

diff --git a/packages/wrangler/pages/functions/template-worker.ts b/packages/wrangler/pages/functions/template-worker.ts
@@ -22,7 +22,7 @@ declare type PagesFunction<
 
 type RouteHandler = {
   routePath: string;
-  methods: HTTPMethod[];
+  method?: HTTPMethod;
   modules: PagesFunction[];
   middlewares: PagesFunction[];
 };
@@ -47,10 +47,7 @@ function* executeRequest(request: Request, _env: FetchEnv) {
 
   // First, iterate through the routes (backwards) and execute "middlewares" on partial route matches
   for (const route of [...routes].reverse()) {
-    if (
-      route.methods.length &&
-      !route.methods.includes(request.method as HTTPMethod)
-    ) {
+    if (route.method && route.method !== request.method) {
       continue;
     }
 
@@ -68,10 +65,7 @@ function* executeRequest(request: Request, _env: FetchEnv) {
 
   // Then look for the first exact route match and execute its "modules"
   for (const route of routes) {
-    if (
-      route.methods.length &&
-      !route.methods.includes(request.method as HTTPMethod)
-    ) {
+    if (route.method && route.method !== request.method) {
       continue;
     }