From 069e3080c79009ac058c40879582535f0e2fe791 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Oct 2022 11:51:13 -0400 Subject: [PATCH 1/5] Updates to createStaticHandler for Remix consumption --- packages/router/router.ts | 70 +++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 21 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 73a3b538c4..c35876c82a 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1070,7 +1070,7 @@ export function createRouter(init: RouterInit): Router { // a revalidation interrupting an actionReload) if (!isUninterruptedRevalidation) { revalidatingFetchers.forEach(([key]) => { - const fetcher = state.fetchers.get(key); + let fetcher = state.fetchers.get(key); let revalidatingFetcher: FetcherStates["Loading"] = { state: "loading", data: fetcher && fetcher.data, @@ -1774,6 +1774,9 @@ export function createRouter(init: RouterInit): Router { //#region createStaticHandler //////////////////////////////////////////////////////////////////////////////// +const validActionMethods = new Set(["POST", "PUT", "PATCH", "DELETE"]); +const validRequestMethods = new Set(["GET", "HEAD", ...validActionMethods]); + export function unstable_createStaticHandler( routes: AgnosticRouteObject[] ): StaticHandler { @@ -1810,7 +1813,25 @@ export function unstable_createStaticHandler( let location = createLocation("", createPath(url), null, "default"); let matches = matchRoutes(dataRoutes, location); - if (!matches) { + if (!validRequestMethods.has(request.method)) { + let { + matches: methodNotAllowedMatches, + route, + error, + } = getMethodNotAllowedMatches(dataRoutes); + return { + location, + matches: methodNotAllowedMatches, + loaderData: {}, + actionData: null, + errors: { + [route.id]: error, + }, + statusCode: error.status, + loaderHeaders: {}, + actionHeaders: {}, + }; + } else if (!matches) { let { matches: notFoundMatches, route, @@ -1824,7 +1845,7 @@ export function unstable_createStaticHandler( errors: { [route.id]: error, }, - statusCode: 404, + statusCode: error.status, loaderHeaders: {}, actionHeaders: {}, }; @@ -1863,7 +1884,12 @@ export function unstable_createStaticHandler( let location = createLocation("", createPath(url), null, "default"); let matches = matchRoutes(dataRoutes, location); - if (!matches) { + if (!validRequestMethods.has(request.method)) { + throw createRouterErrorResponse(null, { + status: 405, + statusText: "Method Not Allowed", + }); + } else if (!matches) { throw createRouterErrorResponse(null, { status: 404, statusText: "Not Found", @@ -1906,17 +1932,13 @@ export function unstable_createStaticHandler( matches: AgnosticDataRouteMatch[], isRouteRequest: boolean ): Promise | Response> { - invariant( - request.method !== "HEAD", - "query()/queryRoute() do not support HEAD requests" - ); invariant( request.signal, "query()/queryRoute() requests must contain an AbortController signal" ); try { - if (request.method !== "GET") { + if (validActionMethods.has(request.method)) { let result = await submit( request, matches, @@ -1963,7 +1985,7 @@ export function unstable_createStaticHandler( if (!actionMatch.route.action) { let href = createServerHref(new URL(request.url)); if (isRouteRequest) { - throw createRouterErrorResponse(`No action found for [${href}]`, { + throw createRouterErrorResponse(null, { status: 405, statusText: "Method Not Allowed", }); @@ -2735,16 +2757,18 @@ function findNearestBoundary( ); } -function getNotFoundMatches(routes: AgnosticDataRouteObject[]): { +function getShortCircuitMatches( + routes: AgnosticDataRouteObject[], + status: number, + statusText: string +): { matches: AgnosticDataRouteMatch[]; route: AgnosticDataRouteObject; error: ErrorResponse; } { // Prefer a root layout route if present, otherwise shim in a route object - let route = routes.find( - (r) => r.index || r.path === "" || r.path === "/" - ) || { - id: "__shim-404-route__", + let route = routes.find((r) => r.index || !r.path || r.path === "/") || { + id: `__shim-${status}-route__`, }; return { @@ -2757,10 +2781,18 @@ function getNotFoundMatches(routes: AgnosticDataRouteObject[]): { }, ], route, - error: new ErrorResponse(404, "Not Found", null), + error: new ErrorResponse(status, statusText, null), }; } +function getNotFoundMatches(routes: AgnosticDataRouteObject[]) { + return getShortCircuitMatches(routes, 404, "Not Found"); +} + +function getMethodNotAllowedMatches(routes: AgnosticDataRouteObject[]) { + return getShortCircuitMatches(routes, 405, "Method Not Allowed"); +} + function getMethodNotAllowedResult(path: Location | string): ErrorResult { let href = typeof path === "string" ? path : createServerHref(path); console.warn( @@ -2770,11 +2802,7 @@ function getMethodNotAllowedResult(path: Location | string): ErrorResult { ); return { type: ResultType.error, - error: new ErrorResponse( - 405, - "Method Not Allowed", - `No action found for [${href}]` - ), + error: new ErrorResponse(405, "Method Not Allowed", ""), }; } From 47584cd350806f6619c44efb1eec199e4c7f5fb5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Oct 2022 11:57:08 -0400 Subject: [PATCH 2/5] unit tests --- packages/router/__tests__/router-test.ts | 67 ++++++++---------------- 1 file changed, 22 insertions(+), 45 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index c159d04892..6530c54667 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -3123,11 +3123,7 @@ describe("a router", () => { formData: createFormData({ gosh: "dang" }), }); expect(t.router.state.errors).toEqual({ - child: new ErrorResponse( - 405, - "Method Not Allowed", - "No action found for [/child]" - ), + child: new ErrorResponse(405, "Method Not Allowed", ""), }); expect(console.warn).toHaveBeenCalled(); spy.mockReset(); @@ -3180,11 +3176,7 @@ describe("a router", () => { }); expect(t.router.state.actionData).toBe(null); expect(t.router.state.errors).toEqual({ - grandchild: new ErrorResponse( - 405, - "Method Not Allowed", - "No action found for [/child/grandchild]" - ), + grandchild: new ErrorResponse(405, "Method Not Allowed", ""), }); }); }); @@ -6381,11 +6373,7 @@ describe("a router", () => { }); expect(A.fetcher).toBe(IDLE_FETCHER); expect(t.router.state.errors).toEqual({ - root: new ErrorResponse( - 405, - "Method Not Allowed", - "No action found for [/]" - ), + root: new ErrorResponse(405, "Method Not Allowed", ""), }); }); @@ -9620,6 +9608,23 @@ describe("a router", () => { }); }); + it("should support document load navigations with HEAD requests", async () => { + let { query } = createStaticHandler(SSR_ROUTES); + let context = await query( + createRequest("/parent/child", { method: "HEAD" }) + ); + expect(context).toMatchObject({ + actionData: null, + loaderData: { + parent: "PARENT LOADER", + child: "CHILD LOADER", + }, + errors: null, + location: { pathname: "/parent/child" }, + matches: [{ route: { id: "parent" } }, { route: { id: "child" } }], + }); + }); + it("should support document load navigations returning responses", async () => { let { query } = createStaticHandler(SSR_ROUTES); let context = await query(createRequest("/parent/json")); @@ -9870,20 +9875,6 @@ describe("a router", () => { expect(e).toMatchInlineSnapshot(`[Error: query() call aborted]`); }); - it("should not support HEAD requests", async () => { - let { query } = createStaticHandler(SSR_ROUTES); - let request = createRequest("/", { method: "head" }); - let e; - try { - await query(request); - } catch (_e) { - e = _e; - } - expect(e).toMatchInlineSnapshot( - `[Error: query()/queryRoute() do not support HEAD requests]` - ); - }); - it("should require a signal on the request", async () => { let { query } = createStaticHandler(SSR_ROUTES); let request = createRequest("/", { signal: undefined }); @@ -9914,7 +9905,7 @@ describe("a router", () => { root: { status: 405, statusText: "Method Not Allowed", - data: "No action found for [/]", + data: "", }, }, matches: [{ route: { id: "root" } }], @@ -10656,20 +10647,6 @@ describe("a router", () => { expect(e).toMatchInlineSnapshot(`[Error: queryRoute() call aborted]`); }); - it("should not support HEAD requests", async () => { - let { queryRoute } = createStaticHandler(SSR_ROUTES); - let request = createRequest("/", { method: "head" }); - let e; - try { - await queryRoute(request, "index"); - } catch (_e) { - e = _e; - } - expect(e).toMatchInlineSnapshot( - `[Error: query()/queryRoute() do not support HEAD requests]` - ); - }); - it("should require a signal on the request", async () => { let { queryRoute } = createStaticHandler(SSR_ROUTES); let request = createRequest("/", { signal: undefined }); @@ -10753,7 +10730,7 @@ describe("a router", () => { expect(data.status).toBe(405); expect(data.statusText).toBe("Method Not Allowed"); expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - expect(await data.text()).toBe("No action found for [/]"); + expect(await data.text()).toBe(""); } /* eslint-enable jest/no-conditional-expect */ }); From 8434506d0f9cbb39123cbb3b6d61bd03be2ab527 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Oct 2022 11:57:33 -0400 Subject: [PATCH 3/5] add changeset --- .changeset/funny-shirts-admire.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/funny-shirts-admire.md diff --git a/.changeset/funny-shirts-admire.md b/.changeset/funny-shirts-admire.md new file mode 100644 index 0000000000..9c3575ce71 --- /dev/null +++ b/.changeset/funny-shirts-admire.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Changes to statis handler for incorporating into Remix" From 1276208f6421cbf89e78d39689c048fdff97ef69 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Oct 2022 12:00:49 -0400 Subject: [PATCH 4/5] one more test for HEAD in queryRoute --- packages/router/__tests__/router-test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 6530c54667..4952980656 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -10291,6 +10291,14 @@ describe("a router", () => { expect(data).toBe("CHILD LOADER"); }); + it("should support HEAD requests", async () => { + let { queryRoute } = createStaticHandler(SSR_ROUTES); + let data = await queryRoute( + createRequest("/parent", { method: "HEAD" }) + ); + expect(data).toBe("PARENT LOADER"); + }); + it("should support singular route load navigations (primitives)", async () => { let { queryRoute } = createStaticHandler(SSR_ROUTES); let data; From 7226afe31991753bf5afd0a10cdc7ae61323935c Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Oct 2022 14:21:42 -0400 Subject: [PATCH 5/5] add tests --- packages/router/__tests__/router-test.ts | 129 +++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 4952980656..8350e48b0f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -2139,6 +2139,31 @@ describe("a router", () => { ]); }); + it("matches root pathless route", () => { + let t = setup({ + routes: [{ id: "root", children: [{ path: "foo" }] }], + }); + + t.navigate("/not-found"); + expect(t.router.state.errors).toEqual({ + root: { + status: 404, + statusText: "Not Found", + data: null, + }, + }); + expect(t.router.state.matches).toMatchObject([ + { + params: {}, + pathname: "", + route: { + id: "root", + children: expect.any(Array), + }, + }, + ]); + }); + it("clears prior loader/action data", async () => { let t = initializeTmTest(); expect(t.router.state.loaderData).toEqual({ @@ -9681,6 +9706,39 @@ describe("a router", () => { }); }); + it("should support alternative submission methods", async () => { + let { query } = createStaticHandler(SSR_ROUTES); + let context; + + let expected = { + actionData: { + child: "CHILD ACTION", + }, + loaderData: { + parent: "PARENT LOADER", + child: "CHILD LOADER", + }, + errors: null, + location: { pathname: "/parent/child" }, + matches: [{ route: { id: "parent" } }, { route: { id: "child" } }], + }; + + context = await query( + createSubmitRequest("/parent/child", { method: "PUT" }) + ); + expect(context).toMatchObject(expected); + + context = await query( + createSubmitRequest("/parent/child", { method: "PATCH" }) + ); + expect(context).toMatchObject(expected); + + context = await query( + createSubmitRequest("/parent/child", { method: "DELETE" }) + ); + expect(context).toMatchObject(expected); + }); + it("should support document submit navigations returning responses", async () => { let { query } = createStaticHandler(SSR_ROUTES); let context = await query(createSubmitRequest("/parent/json")); @@ -9912,6 +9970,29 @@ describe("a router", () => { }); }); + it("should handle unsupported methods with a 405 error", async () => { + let { query } = createStaticHandler([ + { + id: "root", + path: "/", + }, + ]); + let request = createRequest("/", { method: "OPTIONS" }); + let context = await query(request); + expect(context).toMatchObject({ + actionData: null, + loaderData: {}, + errors: { + root: { + status: 405, + statusText: "Method Not Allowed", + data: null, + }, + }, + matches: [{ route: { id: "root" } }], + }); + }); + describe("statusCode", () => { it("should expose a 200 status code by default", async () => { let { query } = createStaticHandler([ @@ -10467,6 +10548,29 @@ describe("a router", () => { expect(data).toBe(""); }); + it("should support alternative submission methods", async () => { + let { queryRoute } = createStaticHandler(SSR_ROUTES); + let data; + + data = await queryRoute( + createSubmitRequest("/parent", { method: "PUT" }), + "parent" + ); + expect(data).toBe("PARENT ACTION"); + + data = await queryRoute( + createSubmitRequest("/parent", { method: "PATCH" }), + "parent" + ); + expect(data).toBe("PARENT ACTION"); + + data = await queryRoute( + createSubmitRequest("/parent", { method: "DELETE" }), + "parent" + ); + expect(data).toBe("PARENT ACTION"); + }); + it("should support singular route submit navigations (Responses)", async () => { /* eslint-disable jest/no-conditional-expect */ let T = setupFlexRouteTest(); @@ -10742,6 +10846,31 @@ describe("a router", () => { } /* eslint-enable jest/no-conditional-expect */ }); + + it("should handle unsupported methods with a 405 Response", async () => { + /* eslint-disable jest/no-conditional-expect */ + let { queryRoute } = createStaticHandler([ + { + id: "root", + path: "/", + }, + ]); + + try { + await queryRoute( + createSubmitRequest("/", { method: "OPTIONS" }), + "root" + ); + expect(false).toBe(true); + } catch (data) { + expect(data instanceof Response).toBe(true); + expect(data.status).toBe(405); + expect(data.statusText).toBe("Method Not Allowed"); + expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); + expect(await data.text()).toBe(""); + } + /* eslint-enable jest/no-conditional-expect */ + }); }); }); });