From 474102b15241dcdcb1bb20dddf7ac3bd59e6d07d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 22 Oct 2024 10:31:44 -0400 Subject: [PATCH] Fix defaultShouldRevalidate when using single fetch (#10139) --- .changeset/twenty-waves-cheer.md | 5 ++ integration/single-fetch-test.ts | 97 +++++++++++++++++++++++++++++ packages/remix-react/components.tsx | 6 +- packages/remix-react/links.ts | 16 +++-- packages/remix-react/routes.tsx | 59 +++++++++++++----- 5 files changed, 160 insertions(+), 23 deletions(-) create mode 100644 .changeset/twenty-waves-cheer.md diff --git a/.changeset/twenty-waves-cheer.md b/.changeset/twenty-waves-cheer.md new file mode 100644 index 00000000000..9778e9412b2 --- /dev/null +++ b/.changeset/twenty-waves-cheer.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +Fix `defaultShouldRevalidate` value when using single fetch diff --git a/integration/single-fetch-test.ts b/integration/single-fetch-test.ts index 10c9c62bf97..60a52495897 100644 --- a/integration/single-fetch-test.ts +++ b/integration/single-fetch-test.ts @@ -2327,6 +2327,103 @@ test.describe("single-fetch", () => { ).toBe(true); }); + test("provides the proper defaultShouldRevalidate value", async ({ + page, + }) => { + let fixture = await createFixture({ + config: { + future: { + v3_singleFetch: true, + }, + }, + files: { + ...files, + "app/routes/_index.tsx": js` + import { Link } from '@remix-run/react'; + export default function Component() { + return Go to /parent/a; + } + `, + "app/routes/parent.tsx": js` + import { Link, Outlet, useLoaderData } from '@remix-run/react'; + 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 '@remix-run/react'; + 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 '@remix-run/react'; + 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/remix-react/components.tsx b/packages/remix-react/components.tsx index d0e315fae2d..3a773fab73b 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -459,9 +459,10 @@ function PrefetchPageLinksImpl({ matches, manifest, location, + future, "data" ), - [page, nextMatches, matches, manifest, location] + [page, nextMatches, matches, manifest, location, future] ); let dataHrefs = React.useMemo(() => { @@ -535,9 +536,10 @@ function PrefetchPageLinksImpl({ matches, manifest, location, + future, "assets" ), - [page, nextMatches, matches, manifest, location] + [page, nextMatches, matches, manifest, location, future] ); let moduleHrefs = React.useMemo( diff --git a/packages/remix-react/links.ts b/packages/remix-react/links.ts index c9d3245b739..03751b44c66 100644 --- a/packages/remix-react/links.ts +++ b/packages/remix-react/links.ts @@ -2,7 +2,7 @@ import type { AgnosticDataRouteMatch } from "@remix-run/router"; import type { Location } from "react-router-dom"; import { parsePath } from "react-router-dom"; -import type { AssetsManifest } from "./entry"; +import type { AssetsManifest, FutureConfig } from "./entry"; import type { RouteModules, RouteModule } from "./routeModules"; import type { EntryRoute } from "./routes"; import { loadRouteModule } from "./routeModules"; @@ -353,6 +353,7 @@ export function getNewMatchesForLinks( currentMatches: AgnosticDataRouteMatch[], manifest: AssetsManifest, location: Location, + future: FutureConfig, mode: "data" | "assets" ): AgnosticDataRouteMatch[] { let path = parsePathPatch(page); @@ -376,7 +377,8 @@ export function getNewMatchesForLinks( // NOTE: keep this mostly up-to-date w/ the transition data diff, but this // version doesn't care about submissions let newMatches = - mode === "data" && location.search !== path.search + mode === "data" && + (future.v3_singleFetch || 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) => { @@ -389,6 +391,12 @@ export function getNewMatchesForLinks( return true; } + // For reused routes on GET navigations, by default: + // - Single fetch always revalidates + // - Multi fetch revalidates if search params changed + let defaultShouldRevalidate = + future.v3_singleFetch || location.search !== path.search; + if (match.route.shouldRevalidate) { let routeChoice = match.route.shouldRevalidate({ currentUrl: new URL( @@ -398,13 +406,13 @@ export function getNewMatchesForLinks( currentParams: currentMatches[0]?.params || {}, nextUrl: new URL(page, window.origin), nextParams: match.params, - defaultShouldRevalidate: true, + defaultShouldRevalidate, }); if (typeof routeChoice === "boolean") { return routeChoice; } } - return true; + return defaultShouldRevalidate; }) : nextMatches.filter((match, index) => { let manifestRoute = manifest.routes[match.route.id]; diff --git a/packages/remix-react/routes.tsx b/packages/remix-react/routes.tsx index 90675e41f07..fc324290863 100644 --- a/packages/remix-react/routes.tsx +++ b/packages/remix-react/routes.tsx @@ -1,5 +1,8 @@ import * as React from "react"; -import type { HydrationState } from "@remix-run/router"; +import type { + HydrationState, + ShouldRevalidateFunctionArgs, +} from "@remix-run/router"; import { UNSAFE_ErrorResponseImpl as ErrorResponse } from "@remix-run/router"; import type { ActionFunctionArgs, @@ -315,13 +318,12 @@ export function createClientRoutes( ...dataRoute, ...getRouteComponents(route, routeModule, isSpaMode), handle: routeModule.handle, - shouldRevalidate: needsRevalidation - ? wrapShouldRevalidateForHdr( - route.id, - routeModule.shouldRevalidate, - needsRevalidation - ) - : routeModule.shouldRevalidate, + shouldRevalidate: getShouldRevalidateFunction( + future, + routeModule, + route.id, + needsRevalidation + ), }); let initialData = initialState?.loaderData?.[route.id]; @@ -474,19 +476,16 @@ 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( + future, + lazyRoute, + route.id, + needsRevalidation + ), handle: lazyRoute.handle, // No need to wrap these in layout since the root route is never // loaded via route.lazy() @@ -511,6 +510,32 @@ export function createClientRoutes( }); } +function getShouldRevalidateFunction( + future: FutureConfig, + 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 (future.v3_singleFetch && 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(