diff --git a/integration/single-fetch-test.ts b/integration/single-fetch-test.ts index dbdceaed8b..39a3a4898e 100644 --- a/integration/single-fetch-test.ts +++ b/integration/single-fetch-test.ts @@ -2108,6 +2108,98 @@ test.describe("single-fetch", () => { ).toBe(true); }); + test("provides the proper defaultShouldRevalidate value", async ({ + page, + }) => { + let fixture = await createFixture({ + files: { + ...files, + "app/routes/_index.tsx": js` + import { Link } from 'react-router'; + export default function Component() { + return Go to /parent/a; + } + `, + "app/routes/parent.tsx": js` + import { Link, Outlet, useLoaderData } from 'react-router'; + let count = 0; + export function loader({ request }) { + return { count: ++count }; + } + export function shouldRevalidate({ defaultShouldRevalidate }) { + return defaultShouldRevalidate; + } + export default function Component() { + return ( + <> +

Parent Count: {useLoaderData().count}

+ Go to /parent/a + Go to /parent/b + + + ); + } + `, + "app/routes/parent.a.tsx": js` + import { useLoaderData } from 'react-router'; + let count = 0; + export function loader({ request }) { + return { count: ++count }; + } + export default function Component() { + return

A Count: {useLoaderData().count}

; + } + `, + "app/routes/parent.b.tsx": js` + import { useLoaderData } from 'react-router'; + let count = 0; + export function loader({ request }) { + return { count: ++count }; + } + export default function Component() { + return

B Count: {useLoaderData().count}

; + } + `, + }, + }); + let appFixture = await createAppFixture(fixture); + let app = new PlaywrightFixture(appFixture, page); + + let urls: string[] = []; + page.on("request", (req) => { + if (req.url().includes(".data")) { + urls.push(req.url()); + } + }); + + await app.goto("/"); + + await app.clickLink("/parent/a"); + await page.waitForSelector("#a"); + expect(await app.getHtml("#parent")).toContain("Parent Count: 1"); + expect(await app.getHtml("#a")).toContain("A Count: 1"); + expect(urls.length).toBe(1); + expect(urls[0].endsWith("/parent/a.data")).toBe(true); + urls = []; + + await app.clickLink("/parent/b"); + await page.waitForSelector("#b"); + expect(await app.getHtml("#parent")).toContain("Parent Count: 2"); + expect(await app.getHtml("#b")).toContain("B Count: 1"); + expect(urls.length).toBe(1); + // Reload the parent route + expect(urls[0].endsWith("/parent/b.data")).toBe(true); + urls = []; + + await app.clickLink("/parent/a"); + await page.waitForSelector("#a"); + expect(await app.getHtml("#parent")).toContain("Parent Count: 3"); + expect(await app.getHtml("#a")).toContain("A Count: 2"); + expect(urls.length).toBe(1); + // Reload the parent route + expect(urls[0].endsWith("/parent/a.data")).toBe(true); + }); + test("does not add a _routes param for routes without loaders", async ({ page, }) => { diff --git a/packages/react-router/lib/dom/ssr/links.ts b/packages/react-router/lib/dom/ssr/links.ts index 80473550fd..50bb073e92 100644 --- a/packages/react-router/lib/dom/ssr/links.ts +++ b/packages/react-router/lib/dom/ssr/links.ts @@ -168,8 +168,6 @@ export function getNewMatchesForLinks( location: Location, mode: "data" | "assets" ): AgnosticDataRouteMatch[] { - let path = parsePathPatch(page); - let isNew = (match: AgnosticDataRouteMatch, index: number) => { if (!currentMatches[index]) return true; return match.route.id !== currentMatches[index].route.id; @@ -186,48 +184,47 @@ export function getNewMatchesForLinks( ); }; - // NOTE: keep this mostly up-to-date w/ the transition data diff, but this + if (mode === "assets") { + return nextMatches.filter( + (match, index) => isNew(match, index) || matchPathChanged(match, index) + ); + } + + // NOTE: keep this mostly up-to-date w/ the router data diff, but this // version doesn't care about submissions - let newMatches = - mode === "data" && location.search !== path.search - ? // this is really similar to stuff in transition.ts, maybe somebody smarter - // than me (or in less of a hurry) can share some of it. You're the best. - nextMatches.filter((match, index) => { - let manifestRoute = manifest.routes[match.route.id]; - if (!manifestRoute.hasLoader) { - return false; - } - - if (isNew(match, index) || matchPathChanged(match, index)) { - return true; - } - - if (match.route.shouldRevalidate) { - let routeChoice = match.route.shouldRevalidate({ - currentUrl: new URL( - location.pathname + location.search + location.hash, - window.origin - ), - currentParams: currentMatches[0]?.params || {}, - nextUrl: new URL(page, window.origin), - nextParams: match.params, - defaultShouldRevalidate: true, - }); - if (typeof routeChoice === "boolean") { - return routeChoice; - } - } - return true; - }) - : nextMatches.filter((match, index) => { - let manifestRoute = manifest.routes[match.route.id]; - return ( - (mode === "assets" || manifestRoute.hasLoader) && - (isNew(match, index) || matchPathChanged(match, index)) - ); + // TODO: this is really similar to stuff in router.ts, maybe somebody smarter + // than me (or in less of a hurry) can share some of it. You're the best. + if (mode === "data") { + return nextMatches.filter((match, index) => { + let manifestRoute = manifest.routes[match.route.id]; + if (!manifestRoute.hasLoader) { + return false; + } + + if (isNew(match, index) || matchPathChanged(match, index)) { + return true; + } + + if (match.route.shouldRevalidate) { + let routeChoice = match.route.shouldRevalidate({ + currentUrl: new URL( + location.pathname + location.search + location.hash, + window.origin + ), + currentParams: currentMatches[0]?.params || {}, + nextUrl: new URL(page, window.origin), + nextParams: match.params, + defaultShouldRevalidate: true, }); + if (typeof routeChoice === "boolean") { + return routeChoice; + } + } + return true; + }); + } - return newMatches; + return []; } export function getModuleLinkHrefs( @@ -320,13 +317,6 @@ function dedupeLinkDescriptors( }, [] as KeyedLinkDescriptor[]); } -// https://github.com/remix-run/history/issues/897 -function parsePathPatch(href: string) { - let path = parsePath(href); - if (path.search === undefined) path.search = ""; - return path; -} - // Detect if this browser supports (or has it enabled). // Originally added to handle the firefox `network.preload` config: // https://bugzilla.mozilla.org/show_bug.cgi?id=1847811 diff --git a/packages/react-router/lib/dom/ssr/routes.tsx b/packages/react-router/lib/dom/ssr/routes.tsx index f4e1c43cf1..d83664a189 100644 --- a/packages/react-router/lib/dom/ssr/routes.tsx +++ b/packages/react-router/lib/dom/ssr/routes.tsx @@ -5,6 +5,7 @@ import type { ActionFunctionArgs, LoaderFunctionArgs, ShouldRevalidateFunction, + ShouldRevalidateFunctionArgs, } from "../../router/utils"; import { ErrorResponseImpl } from "../../router/utils"; import type { RouteModule, RouteModules } from "./routeModules"; @@ -288,13 +289,11 @@ export function createClientRoutes( ...dataRoute, ...getRouteComponents(route, routeModule, isSpaMode), handle: routeModule.handle, - shouldRevalidate: needsRevalidation - ? wrapShouldRevalidateForHdr( - route.id, - routeModule.shouldRevalidate, - needsRevalidation - ) - : routeModule.shouldRevalidate, + shouldRevalidate: getShouldRevalidateFunction( + routeModule, + route.id, + needsRevalidation + ), }); let hasInitialData = @@ -456,19 +455,15 @@ export function createClientRoutes( }); } - if (needsRevalidation) { - lazyRoute.shouldRevalidate = wrapShouldRevalidateForHdr( - route.id, - mod.shouldRevalidate, - needsRevalidation - ); - } - return { ...(lazyRoute.loader ? { loader: lazyRoute.loader } : {}), ...(lazyRoute.action ? { action: lazyRoute.action } : {}), hasErrorBoundary: lazyRoute.hasErrorBoundary, - shouldRevalidate: lazyRoute.shouldRevalidate, + shouldRevalidate: getShouldRevalidateFunction( + lazyRoute, + route.id, + needsRevalidation + ), handle: lazyRoute.handle, // No need to wrap these in layout since the root route is never // loaded via route.lazy() @@ -492,6 +487,31 @@ export function createClientRoutes( }); } +function getShouldRevalidateFunction( + route: Partial, + routeId: string, + needsRevalidation: Set | undefined +) { + // During HDR we force revalidation for updated routes + if (needsRevalidation) { + return wrapShouldRevalidateForHdr( + routeId, + route.shouldRevalidate, + needsRevalidation + ); + } + + // Single fetch revalidates by default, so override the RR default value which + // matches the multi-fetch behavior with `true` + if (route.shouldRevalidate) { + let fn = route.shouldRevalidate; + return (opts: ShouldRevalidateFunctionArgs) => + fn({ ...opts, defaultShouldRevalidate: true }); + } + + return route.shouldRevalidate; +} + // When an HMR / HDR update happens we opt out of all user-defined // revalidation logic and force a revalidation on the first call function wrapShouldRevalidateForHdr(