From b772d47e22179d0dbbafff29f280eefe393ae161 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mika=C3=ABl=20Anzano?= Date: Tue, 11 Oct 2022 22:39:43 +0200 Subject: [PATCH 1/7] fix: respect basename in loaders and actions redirects (#9418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mikaël ANZANO --- contributors.yml | 1 + packages/router/router.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/contributors.yml b/contributors.yml index 82ff65bb5b..b23ea4c66c 100644 --- a/contributors.yml +++ b/contributors.yml @@ -72,6 +72,7 @@ - loun4 - lqze - lukerSpringTree +- manzano78 - marc2332 - markivancho - marvinruder diff --git a/packages/router/router.ts b/packages/router/router.ts index 2e7e495c38..0e3569d9b0 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -27,6 +27,7 @@ import { convertRoutesToDataRoutes, invariant, isRouteErrorResponse, + joinPaths, matchRoutes, } from "./utils"; @@ -1515,6 +1516,17 @@ export function createRouter(init: RouterInit): Router { let redirectHistoryAction = replace === true ? HistoryAction.Replace : HistoryAction.Push; + let { basename = "/" } = init; + + // If we're operating within a basename, prepend it to the pathname prior + // to handing off to history. + if (basename !== "/") { + navigation.location.pathname = + navigation.location.pathname === "/" + ? basename + : joinPaths([basename, navigation.location.pathname]); + } + await startNavigation(redirectHistoryAction, navigation.location, { overrideNavigation: navigation, }); From 19e798ad2ff98dd775c99ad9fc175e46c7cc2ff1 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 12 Oct 2022 15:13:35 -0400 Subject: [PATCH 2/7] fix: support basename and relative routes in redirects --- packages/router/__tests__/router-test.ts | 406 ++++++++++++++++++++++- packages/router/router.ts | 190 +++++++---- packages/router/utils.ts | 32 ++ 3 files changed, 562 insertions(+), 66 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index ad0abca9f9..792c4f3d0d 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -32,7 +32,7 @@ import type { AgnosticRouteObject, TrackedPromise, } from "../utils"; -import { AbortedDeferredError } from "../utils"; +import { AbortedDeferredError, stripBasename } from "../utils"; /////////////////////////////////////////////////////////////////////////////// //#region Types and Utils @@ -588,10 +588,11 @@ function setup({ let promise = new Promise((r) => { invariant(currentRouter, "No currentRouter available"); let unsubscribe = currentRouter.subscribe(() => { - helpers = getNavigationHelpers( - history.createHref(history.location), - navigationId - ); + let popHref = history.createHref(history.location); + if (currentRouter?.basename) { + popHref = stripBasename(popHref, currentRouter.basename) as string; + } + helpers = getNavigationHelpers(popHref, navigationId); unsubscribe(); r(); }); @@ -602,7 +603,11 @@ function setup({ return helpers; } - helpers = getNavigationHelpers(href, navigationId); + let navHref = href; + if (currentRouter.basename) { + navHref = stripBasename(navHref, currentRouter.basename) as string; + } + helpers = getNavigationHelpers(navHref, navigationId); currentRouter.navigate(href, opts); return helpers; } @@ -5000,6 +5005,287 @@ describe("a router", () => { }); }); + describe("redirects", () => { + let REDIRECT_ROUTES: TestRouteObject[] = [ + { + id: "root", + path: "/", + children: [ + { + id: "parent", + path: "parent", + action: true, + loader: true, + children: [ + { + id: "child", + path: "child", + action: true, + loader: true, + children: [ + { + id: "index", + index: true, + action: true, + loader: true, + }, + ], + }, + ], + }, + ], + }, + ]; + + it("applies the basename to redirects returned from loaders", async () => { + let t = setup({ + routes: REDIRECT_ROUTES, + basename: "/base/name", + initialEntries: ["/base/name"], + }); + + let nav1 = await t.navigate("/base/name/parent"); + + let nav2 = await nav1.loaders.parent.redirectReturn("/parent/child"); + await nav2.loaders.parent.resolve("PARENT"); + await nav2.loaders.child.resolve("CHILD"); + await nav2.loaders.index.resolve("INDEX"); + expect(t.router.state).toMatchObject({ + historyAction: "PUSH", + location: { + pathname: "/base/name/parent/child", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT", + child: "CHILD", + index: "INDEX", + }, + errors: null, + }); + expect(t.history.action).toEqual("PUSH"); + expect(t.history.location.pathname).toEqual("/base/name/parent/child"); + }); + + it("supports relative routing in redirects (from parent navigation loader)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let nav1 = await t.navigate("/parent/child"); + + await nav1.loaders.child.resolve("CHILD"); + await nav1.loaders.index.resolve("INDEX"); + await nav1.loaders.parent.redirectReturn(".."); + // No root loader so redirect lands immediately + expect(t.router.state).toMatchObject({ + location: { + pathname: "/", + }, + navigation: IDLE_NAVIGATION, + loaderData: {}, + errors: null, + }); + }); + + it("supports relative routing in redirects (from child navigation loader)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let nav1 = await t.navigate("/parent/child"); + + await nav1.loaders.parent.resolve("PARENT"); + await nav1.loaders.index.resolve("INDEX"); + let nav2 = await nav1.loaders.child.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); + await nav2.loaders.parent.resolve("PARENT 2"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT 2", + }, + errors: null, + }); + }); + + it("supports relative routing in redirects (from index navigation loader)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let nav1 = await t.navigate("/parent/child"); + + await nav1.loaders.parent.resolve("PARENT"); + await nav1.loaders.child.resolve("INDEX"); + let nav2 = await nav1.loaders.index.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); + await nav2.loaders.parent.resolve("PARENT 2"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT 2", + }, + errors: null, + }); + }); + + it("supports relative routing in redirects (from parent fetch loader)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let fetch = await t.fetch("/parent"); + + await fetch.loaders.parent.redirectReturn(".."); + // No root loader so redirect lands immediately + expect(t.router.state).toMatchObject({ + location: { + pathname: "/", + }, + navigation: IDLE_NAVIGATION, + loaderData: {}, + errors: null, + }); + }); + + it("supports relative routing in redirects (from child fetch loader)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let fetch = await t.fetch("/parent/child"); + let nav = await fetch.loaders.child.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); + + await nav.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT", + }, + errors: null, + }); + }); + + it("supports relative routing in redirects (from index fetch loader)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let fetch = await t.fetch("/parent/child?index"); + let nav = await fetch.loaders.index.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); + + await nav.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT", + }, + errors: null, + }); + }); + + it("supports . redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let nav1 = await t.navigate("/parent"); + + let nav2 = await nav1.loaders.parent.redirectReturn( + "./child", + undefined, + undefined, + ["parent", "child", "index"] + ); + await nav2.loaders.parent.resolve("PARENT"); + await nav2.loaders.child.resolve("CHILD"); + await nav2.loaders.index.resolve("INDEX"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent/child", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT", + child: "CHILD", + index: "INDEX", + }, + errors: null, + }); + }); + + it("supports relative routing in navigation action redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let nav1 = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + let nav2 = await nav1.actions.child.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); + await nav2.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT", + }, + errors: null, + }); + }); + + it("supports relative routing in fetch action redirects", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let nav1 = await t.fetch("/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + let nav2 = await nav1.actions.child.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); + await nav2.loaders.parent.resolve("PARENT"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/parent", + }, + navigation: IDLE_NAVIGATION, + loaderData: { + parent: "PARENT", + }, + errors: null, + }); + }); + }); + describe("scroll restoration", () => { it("restores scroll on navigations", async () => { let t = setup({ @@ -9683,10 +9969,56 @@ describe("a router", () => { it("should handle redirect Responses", async () => { let { query } = createStaticHandler(SSR_ROUTES); - let redirect = await query(createRequest("/redirect")); - expect(redirect instanceof Response).toBe(true); - expect((redirect as Response).status).toBe(302); - expect((redirect as Response).headers.get("Location")).toBe("/"); + let response = await query(createRequest("/redirect")); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe("/"); + }); + + it("should handle relative redirect responses (loader)", async () => { + let { query } = createStaticHandler([ + { + path: "/", + children: [ + { + path: "parent", + children: [ + { + path: "child", + loader: () => redirect(".."), + }, + ], + }, + ], + }, + ]); + let response = await query(createRequest("/parent/child")); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe("/parent"); + }); + + it("should handle relative redirect responses (action)", async () => { + let { query } = createStaticHandler([ + { + path: "/", + children: [ + { + path: "parent", + children: [ + { + path: "child", + action: () => redirect(".."), + }, + ], + }, + ], + }, + ]); + let response = await query(createSubmitRequest("/parent/child")); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe("/parent"); }); it("should handle 404 navigations", async () => { @@ -10493,6 +10825,60 @@ describe("a router", () => { expect(data).toBe(""); }); + it("should handle relative redirect responses (loader)", async () => { + let { queryRoute } = createStaticHandler([ + { + path: "/", + children: [ + { + path: "parent", + children: [ + { + id: "child", + path: "child", + loader: () => redirect(".."), + }, + ], + }, + ], + }, + ]); + let response = await queryRoute( + createRequest("/parent/child"), + "child" + ); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe("/parent"); + }); + + it("should handle relative redirect responses (action)", async () => { + let { queryRoute } = createStaticHandler([ + { + path: "/", + children: [ + { + path: "parent", + children: [ + { + id: "child", + path: "child", + action: () => redirect(".."), + }, + ], + }, + ], + }, + ]); + let response = await queryRoute( + createSubmitRequest("/parent/child"), + "child" + ); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe("/parent"); + }); + it("should not unwrap responses returned from loaders", async () => { let response = json({ key: "value" }); let { queryRoute } = createStaticHandler([ diff --git a/packages/router/router.ts b/packages/router/router.ts index 0e3569d9b0..24e32f5d79 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -25,10 +25,12 @@ import { ErrorResponse, ResultType, convertRoutesToDataRoutes, + getPathContributingMatches, invariant, isRouteErrorResponse, joinPaths, matchRoutes, + resolveTo, } from "./utils"; //////////////////////////////////////////////////////////////////////////////// @@ -470,14 +472,25 @@ interface HandleLoadersResult extends ShortCircuitable { } /** - * Tuple of [key, href, DataRouterMatch] for a revalidating fetcher.load() + * Tuple of [key, href, DataRouteMatch, DataRouteMatch[]] for a revalidating + * fetcher.load() */ -type RevalidatingFetcher = [string, string, AgnosticDataRouteMatch]; +type RevalidatingFetcher = [ + string, + string, + AgnosticDataRouteMatch, + AgnosticDataRouteMatch[] +]; /** - * Tuple of [href, DataRouteMatch] for an active fetcher.load() + * Tuple of [href, DataRouteMatch, DataRouteMatch[]] for an active + * fetcher.load() */ -type FetchLoadMatch = [string, AgnosticDataRouteMatch]; +type FetchLoadMatch = [ + string, + AgnosticDataRouteMatch, + AgnosticDataRouteMatch[] +]; export const IDLE_NAVIGATION: NavigationStates["Idle"] = { state: "idle", @@ -940,7 +953,13 @@ export function createRouter(init: RouterInit): Router { if (!actionMatch.route.action) { result = getMethodNotAllowedResult(location); } else { - result = await callLoaderOrAction("action", request, actionMatch); + result = await callLoaderOrAction( + "action", + request, + actionMatch, + matches, + router.basename + ); if (request.signal.aborted) { return { shortCircuited: true }; @@ -1082,6 +1101,7 @@ export function createRouter(init: RouterInit): Router { let { results, loaderResults, fetcherResults } = await callLoadersAndMaybeResolveData( state.matches, + matches, matchesToLoad, revalidatingFetchers, request @@ -1171,14 +1191,14 @@ export function createRouter(init: RouterInit): Router { let match = getTargetMatch(matches, path); if (submission) { - handleFetcherAction(key, routeId, path, match, submission); + handleFetcherAction(key, routeId, path, match, matches, submission); return; } // Store off the match so we can call it's shouldRevalidate on subsequent // revalidations - fetchLoadMatches.set(key, [path, match]); - handleFetcherLoader(key, routeId, path, match); + fetchLoadMatches.set(key, [path, match, matches]); + handleFetcherLoader(key, routeId, path, match, matches); } // Call the action for the matched fetcher.submit(), and then handle redirects, @@ -1188,6 +1208,7 @@ export function createRouter(init: RouterInit): Router { routeId: string, path: string, match: AgnosticDataRouteMatch, + requestMatches: AgnosticDataRouteMatch[], submission: Submission ) { interruptActiveLoads(); @@ -1214,7 +1235,13 @@ export function createRouter(init: RouterInit): Router { let fetchRequest = createRequest(path, abortController.signal, submission); fetchControllers.set(key, abortController); - let actionResult = await callLoaderOrAction("action", fetchRequest, match); + let actionResult = await callLoaderOrAction( + "action", + fetchRequest, + match, + requestMatches, + router.basename + ); if (fetchRequest.signal.aborted) { // We can delete this so long as we weren't aborted by ou our own fetcher @@ -1316,6 +1343,7 @@ export function createRouter(init: RouterInit): Router { let { results, loaderResults, fetcherResults } = await callLoadersAndMaybeResolveData( state.matches, + matches, matchesToLoad, revalidatingFetchers, revalidationRequest @@ -1396,7 +1424,8 @@ export function createRouter(init: RouterInit): Router { key: string, routeId: string, path: string, - match: AgnosticDataRouteMatch + match: AgnosticDataRouteMatch, + matches: AgnosticDataRouteMatch[] ) { let existingFetcher = state.fetchers.get(key); // Put this fetcher into it's loading state @@ -1418,7 +1447,9 @@ export function createRouter(init: RouterInit): Router { let result: DataResult = await callLoaderOrAction( "loader", fetchRequest, - match + match, + matches, + router.basename ); // Deferred isn't supported or fetcher loads, await everything and treat it @@ -1516,16 +1547,6 @@ export function createRouter(init: RouterInit): Router { let redirectHistoryAction = replace === true ? HistoryAction.Replace : HistoryAction.Push; - let { basename = "/" } = init; - - // If we're operating within a basename, prepend it to the pathname prior - // to handing off to history. - if (basename !== "/") { - navigation.location.pathname = - navigation.location.pathname === "/" - ? basename - : joinPaths([basename, navigation.location.pathname]); - } await startNavigation(redirectHistoryAction, navigation.location, { overrideNavigation: navigation, @@ -1534,6 +1555,7 @@ export function createRouter(init: RouterInit): Router { async function callLoadersAndMaybeResolveData( currentMatches: AgnosticDataRouteMatch[], + matches: AgnosticDataRouteMatch[], matchesToLoad: AgnosticDataRouteMatch[], fetchersToLoad: RevalidatingFetcher[], request: Request @@ -1542,9 +1564,17 @@ export function createRouter(init: RouterInit): Router { // then slice off the results into separate arrays so we can handle them // accordingly let results = await Promise.all([ - ...matchesToLoad.map((m) => callLoaderOrAction("loader", request, m)), - ...fetchersToLoad.map(([, href, match]) => - callLoaderOrAction("loader", createRequest(href, request.signal), match) + ...matchesToLoad.map((match) => + callLoaderOrAction("loader", request, match, matches, router.basename) + ), + ...fetchersToLoad.map(([, href, match, fetchMatches]) => + callLoaderOrAction( + "loader", + createRequest(href, request.signal), + match, + fetchMatches, + router.basename + ) ), ]); let loaderResults = results.slice(0, matchesToLoad.length); @@ -1845,7 +1875,7 @@ export function unstable_createStaticHandler( "query()/queryRoute() requests must contain an AbortController signal" ); - let { location, matches, shortCircuitState } = matchRequest( + let { location, matches, routeMatch, shortCircuitState } = matchRequest( request, routeId ); @@ -1859,13 +1889,13 @@ export function unstable_createStaticHandler( let result = await submit( request, matches, - getTargetMatch(matches, location), + routeMatch || getTargetMatch(matches, location), routeId != null ); return { location, result }; } - let result = await loadRouteData(request, matches, routeId != null); + let result = await loadRouteData(request, matches, routeMatch); return { location, result: { @@ -1897,6 +1927,8 @@ export function unstable_createStaticHandler( "action", request, actionMatch, + matches, + undefined, // Basename not currently supported in static handlers true, isRouteRequest ); @@ -1909,7 +1941,7 @@ export function unstable_createStaticHandler( if (isRedirectResult(result)) { // Uhhhh - this should never happen, we should always throw these from - // calLoaderOrAction, but the type narrowing here keeps TS happy and we + // callLoaderOrAction, but the type narrowing here keeps TS happy and we // can get back on the "throw all redirect responses" train here should // this ever happen :/ throw new Response(null, { @@ -1959,7 +1991,7 @@ export function unstable_createStaticHandler( // Store off the pending error - we use it to determine which loaders // to call and will commit it when we complete the navigation let boundaryMatch = findNearestBoundary(matches, actionMatch.route.id); - let context = await loadRouteData(request, matches, isRouteRequest, { + let context = await loadRouteData(request, matches, undefined, { [boundaryMatch.route.id]: result.error, }); @@ -1976,7 +2008,7 @@ export function unstable_createStaticHandler( }; } - let context = await loadRouteData(request, matches, isRouteRequest); + let context = await loadRouteData(request, matches); return { ...context, @@ -1994,16 +2026,20 @@ export function unstable_createStaticHandler( async function loadRouteData( request: Request, matches: AgnosticDataRouteMatch[], - isRouteRequest: boolean, + routeMatch?: AgnosticDataRouteMatch, pendingActionError?: RouteData ): Promise< | Omit | Response > { - let matchesToLoad = getLoaderMatchesUntilBoundary( - matches, - Object.keys(pendingActionError || {})[0] - ).filter((m) => m.route.loader); + let isRouteRequest = routeMatch != null; + let requestMatches = routeMatch + ? [routeMatch] + : getLoaderMatchesUntilBoundary( + matches, + Object.keys(pendingActionError || {})[0] + ); + let matchesToLoad = requestMatches.filter((m) => m.route.loader); // Short circuit if we have no loaders to run if (matchesToLoad.length === 0) { @@ -2017,8 +2053,16 @@ export function unstable_createStaticHandler( } let results = await Promise.all([ - ...matchesToLoad.map((m) => - callLoaderOrAction("loader", request, m, true, isRouteRequest) + ...matchesToLoad.map((match) => + callLoaderOrAction( + "loader", + request, + match, + matches, + undefined, // Basename not currently supported in static handlers + true, + isRouteRequest + ) ), ]); @@ -2061,9 +2105,6 @@ export function unstable_createStaticHandler( let url = new URL(req.url); let location = createLocation("", createPath(url), null, "default"); let matches = matchRoutes(dataRoutes, location); - if (matches && routeId) { - matches = matches.filter((m) => m.route.id === routeId); - } // Short circuit with a 404 if we match nothing if (!matches) { @@ -2089,7 +2130,16 @@ export function unstable_createStaticHandler( }; } - return { location, matches }; + let routeMatch: AgnosticDataRouteMatch | undefined = undefined; + if (routeId) { + routeMatch = matches.find((m) => m.route.id === routeId); + invariant( + routeMatch, + `queryRoute could not find route "${routeId}" in matches for path ${req.url}` + ); + } + + return { location, matches, routeMatch }; } return { @@ -2263,10 +2313,10 @@ function getMatchesToLoad( // Pick fetcher.loads that need to be revalidated let revalidatingFetchers: RevalidatingFetcher[] = []; fetchLoadMatches && - fetchLoadMatches.forEach(([href, match], key) => { + fetchLoadMatches.forEach(([href, match, fetchMatches], key) => { // This fetcher was cancelled from a prior action submission - force reload if (cancelledFetcherLoads.includes(key)) { - revalidatingFetchers.push([key, href, match]); + revalidatingFetchers.push([key, href, match, fetchMatches]); } else if (isRevalidationRequired) { let shouldRevalidate = shouldRevalidateLoader( href, @@ -2278,7 +2328,7 @@ function getMatchesToLoad( actionResult ); if (shouldRevalidate) { - revalidatingFetchers.push([key, href, match]); + revalidatingFetchers.push([key, href, match, fetchMatches]); } } }); @@ -2372,7 +2422,9 @@ async function callLoaderOrAction( type: "loader" | "action", request: Request, match: AgnosticDataRouteMatch, - skipRedirects: boolean = false, + matches: AgnosticDataRouteMatch[], + basename: string | undefined, + isStaticRequest: boolean = false, isRouteRequest: boolean = false ): Promise { let resultType; @@ -2403,23 +2455,43 @@ async function callLoaderOrAction( } if (result instanceof Response) { - // Process redirects let status = result.status; - let location = result.headers.get("Location"); - // For SSR single-route requests, we want to hand Responses back directly - // without unwrapping - if (isRouteRequest) { - throw result; - } + // Process redirects + if (status >= 300 && status <= 399) { + let location = result.headers.get("Location"); + invariant( + location, + "Redirects returned/thrown from loaders/actions must have a Location header" + ); + + // Support relative routing in redirects + let activeMatches = matches.slice(0, matches.indexOf(match) + 1); + let routePathnames = getPathContributingMatches(activeMatches).map( + (match) => match.pathnameBase + ); + let requestPath = createURL(request.url).pathname; + location = resolveTo(location, routePathnames, requestPath).pathname; + invariant( + location, + `Unable to resolve redirect location: ${result.headers.get("Location")}` + ); + + // Prepend the basename to the redirect location if we have one + if (basename) { + let path = createURL(location).pathname; + location = path === "/" ? basename : joinPaths([basename, path]); + } - if (status >= 300 && status <= 399 && location != null) { - // Don't process redirects in the router during SSR document requests. + // Don't process redirects in the router during static requests requests. // Instead, throw the Response and let the server handle it with an HTTP - // redirect - if (skipRedirects) { + // redirect. We also update the Location header in place in this flow so + // basename and relative routing is taken into account + if (isStaticRequest) { + result.headers.set("Location", location); throw result; } + return { type: ResultType.redirect, status, @@ -2428,6 +2500,12 @@ async function callLoaderOrAction( }; } + // For static single-route requests, we want to hand any other non-Redirect + // Responses back directly without unwrapping + if (isRouteRequest) { + throw result; + } + let data: any; let contentType = result.headers.get("Content-Type"); if (contentType && contentType.startsWith("application/json")) { diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 7fe1cec1df..6994ba152b 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -835,6 +835,38 @@ function getInvalidPathError( ); } +/** + * When processing relative navigation we want to ignore ancestor routes that + * do not contribute to the path, such that index/pathless layout routes don't + * interfere. + * + * For example, when moving a route element into an index route and/or a + * pathless layout route, relative link behavior contained within should stay + * the same. Both of the following examples should link back to the root: + * + * + * + * + * + * + * + * }> // <-- Does not contribute + * // <-- Does not contribute + * + * + */ +export function getPathContributingMatches< + T extends AgnosticRouteMatch = AgnosticRouteMatch +>(matches: T[]) { + return matches.filter( + (match, index) => + index === 0 || + (!match.route.index && + match.pathnameBase !== matches[index - 1].pathnameBase) + ); +} + /** * @private */ From 03a1177f564cf61095e9537efd9c76b81a52b0c3 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 12 Oct 2022 15:39:33 -0400 Subject: [PATCH 3/7] ci: add tests for data memory router --- .../__tests__/data-memory-router-test.tsx | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx index 55a809fdc4..e6e57ae87b 100644 --- a/packages/react-router/__tests__/data-memory-router-test.tsx +++ b/packages/react-router/__tests__/data-memory-router-test.tsx @@ -9,6 +9,7 @@ import { } from "@testing-library/react"; import "@testing-library/jest-dom"; import type { FormMethod, Router, RouterInit } from "@remix-run/router"; +import { joinPaths } from "@remix-run/router"; import type { RouteObject } from "react-router"; import { Await, @@ -20,6 +21,7 @@ import { createMemoryRouter, createRoutesFromElements, defer, + redirect, useActionData, useAsyncError, useAsyncValue, @@ -158,6 +160,116 @@ describe("", () => { `); }); + it("prepends basename to loader/action redirects", async () => { + let { container } = render( + + }> + redirect("/other")} /> + Other} /> + + + ); + + function Root() { + return ( + <> + Link to thing + + + ); + } + + expect(getHtml(container)).toMatchInlineSnapshot(` + "" + `); + + fireEvent.click(screen.getByText("Link to thing")); + await waitFor(() => screen.getByText("Other")); + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+ + Link to thing + +

+ Other +

+
" + `); + }); + + it("supports relative routing in loader/action redirects", async () => { + let { container } = render( + + }> + }> + redirect("../other")} /> + Other} /> + + + + ); + + function Root() { + return ( + <> + Link to child + + + ); + } + + function Parent() { + return ( + <> +

Parent

+ + + ); + } + + expect(getHtml(container)).toMatchInlineSnapshot(` + "" + `); + + fireEvent.click(screen.getByText("Link to child")); + await waitFor(() => screen.getByText("Parent")); + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+ + Link to child + +

+ Parent +

+

+ Other +

+
" + `); + }); + it("renders with hydration data", async () => { let { container } = render( { event.preventDefault(); From 27944044d53042b087222fa01e67612dbfaca543 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 12 Oct 2022 15:45:33 -0400 Subject: [PATCH 4/7] add changeset --- .changeset/smart-ants-decide.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/smart-ants-decide.md diff --git a/.changeset/smart-ants-decide.md b/.changeset/smart-ants-decide.md new file mode 100644 index 0000000000..ea14d2c312 --- /dev/null +++ b/.changeset/smart-ants-decide.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Support basename and relative routing in loader/action redirects From 6d23d9b48e9898bb0c33235fbc9bda51f9fa446b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 12 Oct 2022 15:46:16 -0400 Subject: [PATCH 5/7] Bump bundle threshold --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c60f250387..f7a0172e09 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.js": { - "none": "100 kB" + "none": "103 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB" From df928299b95a70c6b629edc79cc485b3b873c03b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 12 Oct 2022 16:01:34 -0400 Subject: [PATCH 6/7] convert invariant to 404 for missing routeId --- packages/router/__tests__/router-test.ts | 32 +++++++++++++++++++++++- packages/router/router.ts | 15 ++++------- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 792c4f3d0d..27a0535b72 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -10987,7 +10987,37 @@ describe("a router", () => { ); }); - it("should handle not found action submissions with a 405 Response", async () => { + it("should handle not found loads with a 404 Response", async () => { + let { queryRoute } = createStaticHandler([ + { + id: "root", + path: "/", + }, + ]); + let request = createRequest("/"); + let data = await queryRoute(request, "blah"); + expect(data instanceof Response).toBe(true); + expect(data.status).toBe(404); + expect(data.statusText).toBe("Not Found"); + expect(await data.text()).toBe(""); + }); + + it("should handle not found submits with a 404 Response", async () => { + let { queryRoute } = createStaticHandler([ + { + id: "root", + path: "/", + }, + ]); + let request = createSubmitRequest("/"); + let data = await queryRoute(request, "blah"); + expect(data instanceof Response).toBe(true); + expect(data.status).toBe(404); + expect(data.statusText).toBe("Not Found"); + expect(await data.text()).toBe(""); + }); + + it("should handle missing actions with a 405 Response", async () => { let { queryRoute } = createStaticHandler([ { id: "root", diff --git a/packages/router/router.ts b/packages/router/router.ts index 24e32f5d79..9dea9fffd4 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2105,9 +2105,13 @@ export function unstable_createStaticHandler( let url = new URL(req.url); let location = createLocation("", createPath(url), null, "default"); let matches = matchRoutes(dataRoutes, location); + let routeMatch: AgnosticDataRouteMatch | undefined = undefined; + if (matches && routeId) { + routeMatch = matches.find((m) => m.route.id === routeId); + } // Short circuit with a 404 if we match nothing - if (!matches) { + if (!matches || (routeId && !routeMatch)) { let { matches: notFoundMatches, route, @@ -2130,15 +2134,6 @@ export function unstable_createStaticHandler( }; } - let routeMatch: AgnosticDataRouteMatch | undefined = undefined; - if (routeId) { - routeMatch = matches.find((m) => m.route.id === routeId); - invariant( - routeMatch, - `queryRoute could not find route "${routeId}" in matches for path ${req.url}` - ); - } - return { location, matches, routeMatch }; } From d00e5bc3510b97413c81fdb263934ff02bd529e1 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Oct 2022 17:39:41 -0400 Subject: [PATCH 7/7] Bundle bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 866c02ec24..710c09c033 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.js": { - "none": "105 kB" + "none": "106 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB"