diff --git a/.changeset/friendly-melons-laugh.md b/.changeset/friendly-melons-laugh.md new file mode 100644 index 000000000000..b2c292fc11e1 --- /dev/null +++ b/.changeset/friendly-melons-laugh.md @@ -0,0 +1,13 @@ +--- +"wrangler": patch +--- + +fix: generate valid URL route paths for pages on Windows + +Previously route paths were manipulated by file-system path utilities. +On Windows this resulted in URLs that had backslashes, which are invalid for such URLs. + +Fixes #51 +Closes #235 +Closes #330 +Closes #327 diff --git a/packages/wrangler/pages/functions/filepath-routing.test.ts b/packages/wrangler/pages/functions/filepath-routing.test.ts index d4dfcbe9a995..21e2b377b140 100644 --- a/packages/wrangler/pages/functions/filepath-routing.test.ts +++ b/packages/wrangler/pages/functions/filepath-routing.test.ts @@ -1,39 +1,47 @@ +import { toUrlPath } from "../../src/paths"; import { compareRoutes } from "./filepath-routing"; describe("compareRoutes()", () => { + const url = toUrlPath; test("routes / last", () => { - expect(compareRoutes("/", "/foo")).toBeGreaterThanOrEqual(1); - expect(compareRoutes("/", "/:foo")).toBeGreaterThanOrEqual(1); - expect(compareRoutes("/", "/:foo*")).toBeGreaterThanOrEqual(1); + expect(compareRoutes([url("/")], [url("/foo")])).toBeGreaterThanOrEqual(1); + expect(compareRoutes([url("/")], [url("/:foo")])).toBeGreaterThanOrEqual(1); + expect(compareRoutes([url("/")], [url("/:foo*")])).toBeGreaterThanOrEqual( + 1 + ); }); test("routes with fewer segments come after those with more segments", () => { - expect(compareRoutes("/foo", "/foo/bar")).toBeGreaterThanOrEqual(1); - expect(compareRoutes("/foo", "/foo/bar/cat")).toBeGreaterThanOrEqual(1); + expect( + compareRoutes([url("/foo")], [url("/foo/bar")]) + ).toBeGreaterThanOrEqual(1); + expect( + compareRoutes([url("/foo")], [url("/foo/bar/cat")]) + ).toBeGreaterThanOrEqual(1); }); test("routes with wildcard segments come after those without", () => { - expect(compareRoutes("/:foo*", "/foo")).toBe(1); - expect(compareRoutes("/:foo*", "/:foo")).toBe(1); + expect(compareRoutes([url("/:foo*")], [url("/foo")])).toBe(1); + expect(compareRoutes([url("/:foo*")], [url("/:foo")])).toBe(1); }); test("routes with dynamic segments come after those without", () => { - expect(compareRoutes("/:foo", "/foo")).toBe(1); + expect(compareRoutes([url("/:foo")], [url("/foo")])).toBe(1); }); - test("routes with dynamic segments occuring earlier come after those with dynamic segments in later positions", () => { - expect(compareRoutes("/foo/:id/bar", "/foo/bar/:id")).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); }); test("routes with no HTTP method come after those specifying a method", () => { - expect(compareRoutes("/foo", "GET /foo")).toBe(1); + expect(compareRoutes([url("/foo")], [url("/foo"), "GET"])).toBe(1); }); test("two equal routes are sorted according to their original position in the list", () => { - expect(compareRoutes("GET /foo", "GET /foo")).toBe(0); + expect(compareRoutes([url("/foo"), "GET"], [url("/foo"), "GET"])).toBe(0); }); test("it returns -1 if the first argument should appear first in the list", () => { - expect(compareRoutes("GET /foo", "/foo")).toBe(-1); + expect(compareRoutes([url("/foo"), "GET"], [url("/foo")])).toBe(-1); }); }); diff --git a/packages/wrangler/pages/functions/filepath-routing.ts b/packages/wrangler/pages/functions/filepath-routing.ts index fe4540cbc299..c8dd32253898 100644 --- a/packages/wrangler/pages/functions/filepath-routing.ts +++ b/packages/wrangler/pages/functions/filepath-routing.ts @@ -3,26 +3,31 @@ import path from "node:path"; import * as acorn from "acorn"; 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 { ExportNamedDeclaration, Identifier } from "estree"; type Arguments = { baseDir: string; - baseURL: 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: [string, RouteConfig][] = []; + let routeEntries: [RouteKey, RouteConfig][] = []; if (!baseURL.startsWith("/")) { - baseURL = `/${baseURL}`; + baseURL = `/${baseURL}` as UrlPath; } if (baseURL.endsWith("/")) { - baseURL = baseURL.slice(0, -1); + baseURL = baseURL.slice(0, -1) as UrlPath; } await forEachFile(baseDir, async (filepath) => { @@ -108,12 +113,14 @@ export async function generateConfigFromFileTree({ routePath = routePath.replace(/\[\[(.+)]]/g, ":$1*"); // transform [[id]] => :id* routePath = routePath.replace(/\[(.+)]/g, ":$1"); // transform [id] => :id - if (method) { - routePath = `${method.toUpperCase()} ${routePath}`; - } + const routeUrlPath = toUrlPath(routePath); + const routeKey: RouteKey = [ + routeUrlPath, + method ? (method.toUpperCase() as Method) : undefined, + ]; routeEntries.push([ - routePath, + routeKey, { [isMiddlewareFile ? "middleware" : "module"]: [ `${path.relative(baseDir, filepath)}:${exportName}`, @@ -146,7 +153,7 @@ export async function generateConfigFromFileTree({ [] ); - routeEntries.sort(([pathA], [pathB]) => compareRoutes(pathA, pathB)); + routeEntries.sort(([a], [b]) => compareRoutes(a, b)); return { routes: Object.fromEntries(routeEntries) } as Config; } @@ -154,20 +161,16 @@ export async function generateConfigFromFileTree({ // 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(a: string, b: string) { - function parseRoutePath(routePath: string): [string | null, string[]] { - const parts = routePath.split(" ", 2); - // split() will guarantee at least one element. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const segmentedPath = parts.pop()!; - const method = parts.pop() ?? null; - - const segments = segmentedPath.slice(1).split("/").filter(Boolean); - return [method, segments]; +export function compareRoutes( + [routePathA, methodA]: RouteKey, + [routePathB, methodB]: RouteKey +) { + function parseRoutePath(routePath: UrlPath): string[] { + return routePath.slice(1).split("/").filter(Boolean); } - const [methodA, segmentsA] = parseRoutePath(a); - const [methodB, segmentsB] = parseRoutePath(b); + const segmentsA = parseRoutePath(routePathA); + const segmentsB = parseRoutePath(routePathB); // sort routes with fewer segments after those with more segments if (segmentsA.length !== segmentsB.length) { @@ -193,8 +196,8 @@ export function compareRoutes(a: string, b: string) { if (methodA && !methodB) return -1; if (!methodA && methodB) return 1; - // all else equal, just sort them lexicographically - return a.localeCompare(b); + // all else equal, just sort the paths lexicographically + return routePathA.localeCompare(routePathB); } async function forEachFile( diff --git a/packages/wrangler/pages/functions/routes.ts b/packages/wrangler/pages/functions/routes.ts index 42dd8197f3a7..f3de7dc79afe 100755 --- a/packages/wrangler/pages/functions/routes.ts +++ b/packages/wrangler/pages/functions/routes.ts @@ -1,6 +1,8 @@ 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"; export const HTTP_METHODS = [ "HEAD", @@ -19,7 +21,7 @@ export function isHTTPMethod( } export type RoutesCollection = Array<{ - routePath: string; + routePath: UrlPath; methods: HTTPMethod[]; modules: string[]; middlewares: string[]; @@ -31,7 +33,7 @@ export type Config = { }; export type RoutesConfig = { - [route: string]: RouteConfig; + [route: UrlPath]: RouteConfig; }; export type RouteConfig = { @@ -122,7 +124,7 @@ export function parseConfig(config: Config, baseDir: string) { } routes.push({ - routePath, + routePath: toUrlPath(routePath), methods: _methods.split("|").filter(isHTTPMethod), middlewares: parseModuleIdentifiers(props.middleware), modules: parseModuleIdentifiers(props.module), diff --git a/packages/wrangler/src/pages.tsx b/packages/wrangler/src/pages.tsx index 75667b23a27f..da0fb5052979 100644 --- a/packages/wrangler/src/pages.tsx +++ b/packages/wrangler/src/pages.tsx @@ -11,6 +11,7 @@ import open from "open"; import { buildWorker } from "../pages/functions/buildWorker"; import { generateConfigFromFileTree } from "../pages/functions/filepath-routing"; import { writeRoutesModule } from "../pages/functions/routes"; +import { toUrlPath } from "./paths"; import type { Config } from "../pages/functions/routes"; import type { Headers, Request, fetch } from "@miniflare/core"; import type { BuildResult } from "esbuild"; @@ -665,16 +666,17 @@ async function buildFunctions({ ); const routesModule = join(tmpdir(), "./functionsRoutes.mjs"); + const baseURL = toUrlPath("/"); const config: Config = await generateConfigFromFileTree({ baseDir: functionsDirectory, - baseURL: "/", + baseURL, }); if (outputConfigPath) { writeFileSync( outputConfigPath, - JSON.stringify({ ...config, baseURL: "/" }, null, 2) + JSON.stringify({ ...config, baseURL }, null, 2) ); } diff --git a/packages/wrangler/src/paths.ts b/packages/wrangler/src/paths.ts new file mode 100644 index 000000000000..e61db3fb6c6e --- /dev/null +++ b/packages/wrangler/src/paths.ts @@ -0,0 +1,26 @@ +import { assert } from "console"; + +type DiscriminatedPath = string & { + _discriminator: Discriminator; +}; + +/** + * A branded string that expects to be URL compatible. + * + * Require this type when you want callers to ensure that they have converted file-path strings into URL-safe paths. + */ +export type UrlPath = DiscriminatedPath<"UrlPath">; + +/** + * Convert a file-path string to a URL-path string. + * + * Use this helper to convert a `string` to a `UrlPath` when it is not clear whether the string needs normalizing. + * Replaces all back-slashes with forward-slashes, and throws an error if the path contains a drive letter (e.g. `C:`). + */ +export function toUrlPath(path: string): UrlPath { + assert( + !/^[a-z]:/i.test(path), + "Tried to convert a Windows file path with a drive to a URL path." + ); + return path.replace(/\\/g, "/") as UrlPath; +}