diff --git a/.changeset/stale-coats-smoke.md b/.changeset/stale-coats-smoke.md new file mode 100644 index 0000000000..68cd78074e --- /dev/null +++ b/.changeset/stale-coats-smoke.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Preserve the HTTP method on 307/308 redirects diff --git a/package.json b/package.json index 1f29d2697a..d359a2a285 100644 --- a/package.json +++ b/package.json @@ -119,7 +119,7 @@ "none": "10.5 kB" }, "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": { - "none": "16 kB" + "none": "16.5 kB" } } } diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index c13d5a418b..fbfe12e933 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -365,6 +365,11 @@ function setup({ let redirectNavigationId = ++guid; activeLoaderType = "navigation"; activeLoaderNavigationId = redirectNavigationId; + if (status === 307 || status === 308) { + activeActionType = "navigation"; + activeActionNavigationId = redirectNavigationId; + } + let helpers = getNavigationHelpers(href, redirectNavigationId); // Since a redirect kicks off and awaits a new navigation we can't shim @@ -3966,6 +3971,86 @@ describe("a router", () => { }); }); + it("returns a 405 error if attempting to submit with method=HEAD", async () => { + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { + loaderData: { + root: "ROOT DATA", + index: "INDEX DATA", + }, + }, + }); + + let formData = new FormData(); + formData.append( + "blob", + new Blob(["

Some html file contents

"], { + type: "text/html", + }) + ); + + await t.navigate("/tasks", { + // @ts-expect-error + formMethod: "head", + formData: formData, + }); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.location).toMatchObject({ + pathname: "/tasks", + search: "", + }); + expect(t.router.state.errors).toEqual({ + tasks: new ErrorResponse( + 405, + "Method Not Allowed", + new Error('Invalid request method "HEAD"'), + true + ), + }); + }); + + it("returns a 405 error if attempting to submit with method=OPTIONS", async () => { + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { + loaderData: { + root: "ROOT DATA", + index: "INDEX DATA", + }, + }, + }); + + let formData = new FormData(); + formData.append( + "blob", + new Blob(["

Some html file contents

"], { + type: "text/html", + }) + ); + + await t.navigate("/tasks", { + // @ts-expect-error + formMethod: "options", + formData: formData, + }); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.location).toMatchObject({ + pathname: "/tasks", + search: "", + }); + expect(t.router.state.errors).toEqual({ + tasks: new ErrorResponse( + 405, + "Method Not Allowed", + new Error('Invalid request method "OPTIONS"'), + true + ), + }); + }); + it("runs loaders above the boundary for 400 errors if binary data is attempted to be submitted using formMethod=GET", async () => { let t = setup({ routes: [ @@ -5515,6 +5600,173 @@ describe("a router", () => { window.location = oldLocation; } }); + + describe("redirect status code handling", () => { + it("should not treat 300 as a redirect", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent"); + await A.loaders.parent.redirectReturn("/idk", 300); + expect(t.router.state).toMatchObject({ + loaderData: {}, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + }); + + it("should not preserve the method on 301 redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + // Triggers a GET redirect + let B = await A.actions.child.redirectReturn("/parent", 301); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + loaderData: { + parent: "PARENT", + }, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + }); + + it("should not preserve the method on 302 redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + // Triggers a GET redirect + let B = await A.actions.child.redirectReturn("/parent", 302); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + loaderData: { + parent: "PARENT", + }, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + }); + + it("should not preserve the method on 303 redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + // Triggers a GET redirect + let B = await A.actions.child.redirectReturn("/parent", 303); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + loaderData: { + parent: "PARENT", + }, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + }); + + it("should not treat 304 as a redirect", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent"); + await A.loaders.parent.resolve(new Response(null, { status: 304 })); + expect(t.router.state).toMatchObject({ + loaderData: {}, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + }); + + it("should preserve the method on 307 redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + // Triggers a POST redirect + let B = await A.actions.child.redirectReturn("/parent", 307); + await B.actions.parent.resolve("PARENT ACTION"); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + actionData: { + parent: "PARENT ACTION", + }, + loaderData: { + parent: "PARENT", + }, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + + let request = B.actions.parent.stub.mock.calls[0][0].request; + expect(request.method).toBe("POST"); + let fd = await request.formData(); + expect(Array.from(fd.entries())).toEqual([["key", "value"]]); + }); + + it("should preserve the method on 308 redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + // Triggers a POST redirect + let B = await A.actions.child.redirectReturn("/parent", 308); + await B.actions.parent.resolve("PARENT ACTION"); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + actionData: { + parent: "PARENT ACTION", + }, + loaderData: { + parent: "PARENT", + }, + location: { + pathname: "/parent", + }, + navigation: { + state: "idle", + }, + }); + + let request = B.actions.parent.stub.mock.calls[0][0].request; + expect(request.method).toBe("POST"); + let fd = await request.formData(); + expect(Array.from(fd.entries())).toEqual([["key", "value"]]); + }); + }); }); describe("scroll restoration", () => { @@ -6853,7 +7105,7 @@ describe("a router", () => { 405, "Method Not Allowed", new Error( - 'You made a post request to "/" but did not provide an `action` ' + + 'You made a POST request to "/" but did not provide an `action` ' + 'for route "root", so there is no way to handle the request.' ), true diff --git a/packages/router/router.ts b/packages/router/router.ts index 29ca8c527d..616fd84e8d 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -20,6 +20,7 @@ import type { Submission, SuccessResult, AgnosticRouteMatch, + SubmissionFormMethod, } from "./utils"; import { DeferredData, @@ -514,6 +515,20 @@ interface QueryRouteResponse { response: Response; } +const validActionMethodsArr: SubmissionFormMethod[] = [ + "post", + "put", + "patch", + "delete", +]; +const validActionMethods = new Set(validActionMethodsArr); + +const validRequestMethodsArr: FormMethod[] = ["get", ...validActionMethodsArr]; +const validRequestMethods = new Set(validRequestMethodsArr); + +const redirectStatusCodes = new Set([301, 302, 303, 307, 308]); +const redirectPreserveMethodStatusCodes = new Set([307, 308]); + export const IDLE_NAVIGATION: NavigationStates["Idle"] = { state: "idle", location: undefined, @@ -1014,15 +1029,10 @@ export function createRouter(init: RouterInit): Router { } if (isRedirectResult(result)) { - let redirectNavigation: NavigationStates["Loading"] = { - state: "loading", - location: createLocation(state.location, result.location), - ...submission, - }; await startRedirectNavigation( + state, result, - redirectNavigation, - opts && opts.replace + opts && opts.replace === true ); return { shortCircuited: true }; } @@ -1166,8 +1176,7 @@ export function createRouter(init: RouterInit): Router { // If any loaders returned a redirect Response, start a new REPLACE navigation let redirect = findRedirect(results); if (redirect) { - let redirectNavigation = getLoaderRedirect(state, redirect); - await startRedirectNavigation(redirect, redirectNavigation, replace); + await startRedirectNavigation(state, redirect, replace); return { shortCircuited: true }; } @@ -1318,13 +1327,7 @@ export function createRouter(init: RouterInit): Router { state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); - let redirectNavigation: NavigationStates["Loading"] = { - state: "loading", - location: createLocation(state.location, actionResult.location), - ...submission, - }; - await startRedirectNavigation(actionResult, redirectNavigation); - return; + return startRedirectNavigation(state, actionResult); } // Process any non-redirect errors thrown @@ -1416,9 +1419,7 @@ export function createRouter(init: RouterInit): Router { let redirect = findRedirect(results); if (redirect) { - let redirectNavigation = getLoaderRedirect(state, redirect); - await startRedirectNavigation(redirect, redirectNavigation); - return; + return startRedirectNavigation(state, redirect); } // Process and commit output from loaders @@ -1529,8 +1530,7 @@ export function createRouter(init: RouterInit): Router { // If the loader threw a redirect Response, start a new REPLACE navigation if (isRedirectResult(result)) { - let redirectNavigation = getLoaderRedirect(state, result); - await startRedirectNavigation(result, redirectNavigation); + await startRedirectNavigation(state, result); return; } @@ -1585,15 +1585,17 @@ export function createRouter(init: RouterInit): Router { * the history action from the original navigation (PUSH or REPLACE). */ async function startRedirectNavigation( + state: RouterState, redirect: RedirectResult, - navigation: Navigation, replace?: boolean ) { if (redirect.revalidate) { isRevalidationRequired = true; } + + let redirectLocation = createLocation(state.location, redirect.location); invariant( - navigation.location, + redirectLocation, "Expected a location on the redirect navigation" ); @@ -1613,9 +1615,40 @@ export function createRouter(init: RouterInit): Router { let redirectHistoryAction = replace === true ? HistoryAction.Replace : HistoryAction.Push; - await startNavigation(redirectHistoryAction, navigation.location, { - overrideNavigation: navigation, - }); + let { formMethod, formAction, formEncType, formData } = state.navigation; + + // If this was a 307/308 submission we want to preserve the HTTP method and + // re-submit the POST/PUT/PATCH/DELETE as a submission navigation to the + // redirected location + if ( + redirectPreserveMethodStatusCodes.has(redirect.status) && + formMethod && + isSubmissionMethod(formMethod) && + formEncType && + formData + ) { + await startNavigation(redirectHistoryAction, redirectLocation, { + submission: { + formMethod, + formAction: redirect.location, + formEncType, + formData, + }, + }); + } else { + // Otherwise, we kick off a new loading navigation, preserving the + // submission info for the duration of this navigation + await startNavigation(redirectHistoryAction, redirectLocation, { + overrideNavigation: { + state: "loading", + location: redirectLocation, + formMethod: formMethod || undefined, + formAction: formAction || undefined, + formEncType: formEncType || undefined, + formData: formData || undefined, + }, + }); + } } async function callLoadersAndMaybeResolveData( @@ -1865,9 +1898,6 @@ 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[], opts?: { @@ -1905,11 +1935,13 @@ export function unstable_createStaticHandler( request: Request ): Promise { let url = new URL(request.url); + let method = request.method.toLowerCase(); let location = createLocation("", createPath(url), null, "default"); let matches = matchRoutes(dataRoutes, location, basename); - if (!validRequestMethods.has(request.method)) { - let error = getInternalRouterError(405, { method: request.method }); + // SSR supports HEAD requests while SPA doesn't + if (!isValidMethod(method) && method !== "head") { + let error = getInternalRouterError(405, { method }); let { matches: methodNotAllowedMatches, route } = getShortCircuitMatches(dataRoutes); return { @@ -1977,11 +2009,13 @@ export function unstable_createStaticHandler( */ async function queryRoute(request: Request, routeId?: string): Promise { let url = new URL(request.url); + let method = request.method.toLowerCase(); let location = createLocation("", createPath(url), null, "default"); let matches = matchRoutes(dataRoutes, location, basename); - if (!validRequestMethods.has(request.method)) { - throw getInternalRouterError(405, { method: request.method }); + // SSR supports HEAD requests while SPA doesn't + if (!isValidMethod(method) && method !== "head") { + throw getInternalRouterError(405, { method }); } else if (!matches) { throw getInternalRouterError(404, { pathname: location.pathname }); } @@ -2031,7 +2065,7 @@ export function unstable_createStaticHandler( ); try { - if (validActionMethods.has(request.method)) { + if (isSubmissionMethod(request.method.toLowerCase())) { let result = await submit( request, matches, @@ -2294,6 +2328,12 @@ export function getStaticContextFromError( return newContext; } +function isSubmissionNavigation( + opts: RouterNavigateOptions +): opts is SubmissionNavigateOptions { + return opts != null && "formData" in opts; +} + // Normalize navigation options by converting formMethod=GET formData objects to // URLSearchParams so they behave identically to links with query params function normalizeNavigateOptions( @@ -2308,12 +2348,19 @@ function normalizeNavigateOptions( let path = typeof to === "string" ? to : createPath(to); // Return location verbatim on non-submission navigations - if (!opts || (!("formMethod" in opts) && !("formData" in opts))) { + if (!opts || !isSubmissionNavigation(opts)) { return { path }; } + if (opts.formMethod && !isValidMethod(opts.formMethod)) { + return { + path, + error: getInternalRouterError(405, { method: opts.formMethod }), + }; + } + // Create a Submission on non-GET navigations - if (opts.formMethod != null && opts.formMethod !== "get") { + if (opts.formMethod && isSubmissionMethod(opts.formMethod)) { return { path, submission: { @@ -2326,11 +2373,6 @@ function normalizeNavigateOptions( }; } - // No formData to flatten for GET submission - if (!opts.formData) { - return { path }; - } - // Flatten submission onto URLSearchParams for GET submissions let parsedPath = parsePath(path); try { @@ -2356,22 +2398,6 @@ function normalizeNavigateOptions( return { path: createPath(parsedPath) }; } -function getLoaderRedirect( - state: RouterState, - redirect: RedirectResult -): Navigation { - let { formMethod, formAction, formEncType, formData } = state.navigation; - let navigation: NavigationStates["Loading"] = { - state: "loading", - location: createLocation(state.location, redirect.location), - formMethod: formMethod || undefined, - formAction: formAction || undefined, - formEncType: formEncType || undefined, - formData: formData || undefined, - }; - return navigation; -} - // Filter out all routes below any caught error as they aren't going to // render so we don't need to load them function getLoaderMatchesUntilBoundary( @@ -2581,7 +2607,7 @@ async function callLoaderOrAction( let status = result.status; // Process redirects - if (status >= 300 && status <= 399) { + if (redirectStatusCodes.has(status)) { let location = result.headers.get("Location"); invariant( location, @@ -2932,8 +2958,8 @@ function getInternalRouterError( message?: string; } = {} ) { - let statusText: string; - let errorMessage = message; + let statusText = "Unknown Server Error"; + let errorMessage = "Unknown @remix-run/router error"; if (status === 400) { statusText = "Bad Request"; @@ -2955,15 +2981,12 @@ function getInternalRouterError( statusText = "Method Not Allowed"; if (method && pathname && routeId) { errorMessage = - `You made a ${method} request to "${pathname}" but ` + + `You made a ${method.toUpperCase()} request to "${pathname}" but ` + `did not provide an \`action\` for route "${routeId}", ` + `so there is no way to handle the request.`; - } else { - errorMessage = `Invalid request method "${method}"`; + } else if (method) { + errorMessage = `Invalid request method "${method.toUpperCase()}"`; } - } else { - statusText = "Unknown Server Error"; - errorMessage = "Unknown @remix-run/router error"; } return new ErrorResponse( @@ -3025,6 +3048,14 @@ function isQueryRouteResponse(obj: any): obj is QueryRouteResponse { ); } +function isValidMethod(method: string): method is FormMethod { + return validRequestMethods.has(method as FormMethod); +} + +function isSubmissionMethod(method: string): method is SubmissionFormMethod { + return validActionMethods.has(method as SubmissionFormMethod); +} + async function resolveDeferredResults( currentMatches: AgnosticDataRouteMatch[], matchesToLoad: AgnosticDataRouteMatch[], diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 007605d374..d9ce7d3242 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -62,7 +62,9 @@ export type DataResult = | RedirectResult | ErrorResult; -export type FormMethod = "get" | "post" | "put" | "patch" | "delete"; +export type SubmissionFormMethod = "post" | "put" | "patch" | "delete"; +export type FormMethod = "get" | SubmissionFormMethod; + export type FormEncType = | "application/x-www-form-urlencoded" | "multipart/form-data"; @@ -73,7 +75,7 @@ export type FormEncType = * external consumption */ export interface Submission { - formMethod: Exclude; + formMethod: SubmissionFormMethod; formAction: string; formEncType: FormEncType; formData: FormData;