diff --git a/.changeset/little-taxis-bake.md b/.changeset/little-taxis-bake.md new file mode 100644 index 0000000000..bdf08a4233 --- /dev/null +++ b/.changeset/little-taxis-bake.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix revalidating fetcher `shouldRevalidate` params diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index c7340bbd69..30601a85e1 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -364,13 +364,12 @@ function setup({ } let history = createMemoryHistory({ initialEntries, initialIndex }); - let enhancedRoutes = enhanceRoutes(routes); jest.spyOn(history, "push"); jest.spyOn(history, "replace"); currentRouter = createRouter({ basename, history, - routes: enhancedRoutes, + routes: enhanceRoutes(routes), hydrationData, }).initialize(); @@ -410,21 +409,9 @@ function setup({ // Since a redirect kicks off and awaits a new navigation we can't shim // these _after_ the redirect, so we allow the caller to pass in loader // shims with the redirect - shims.forEach((routeId) => { - invariant( - !helpers.loaders[routeId], - "Can't overwrite existing helpers" - ); - helpers.loaders[routeId] = getRouteHelpers( - routeId, - redirectNavigationId, - (routeId, helpers) => - activeHelpers.set( - `navigation:${redirectNavigationId}:loader:${routeId}`, - helpers - ) - ); - }); + shims.forEach((routeId) => + shimHelper(helpers.loaders, "navigation", "loader", routeId) + ); try { let redirectResponse = redirect(href, { status, headers }); @@ -602,11 +589,13 @@ function setup({ function navigate(n: number): Promise; function navigate( href: string, - opts?: RouterNavigateOptions + opts?: RouterNavigateOptions, + shims?: string[] ): Promise; async function navigate( href: number | string, - opts?: RouterNavigateOptions + opts?: RouterNavigateOptions, + shims?: string[] ): Promise { let navigationId = ++guid; let helpers: NavigationHelpers; @@ -651,6 +640,9 @@ function setup({ navHref = stripBasename(navHref, currentRouter.basename) as string; } helpers = getNavigationHelpers(navHref, navigationId); + shims?.forEach((routeId) => + shimHelper(helpers.loaders, "navigation", "loader", routeId) + ); currentRouter.navigate(href, opts); return helpers; } @@ -683,7 +675,7 @@ function setup({ let routeId = typeof routeIdOrOpts === "string" ? routeIdOrOpts - : String(enhancedRoutes[0].id); + : String(currentRouter?.routes[0].id); opts = typeof keyOrOpts === "object" ? keyOrOpts @@ -1957,19 +1949,40 @@ describe("a router", () => { }); expect(shouldRevalidate.mock.calls.length).toBe(0); - // Normal navigations should not trigger fetcher revalidations + // Normal navigations should trigger fetcher shouldRevalidate with + // defaultShouldRevalidate=false router.navigate("/child"); await tick(); + expect(shouldRevalidate.mock.calls.length).toBe(1); + expect(shouldRevalidate.mock.calls[0][0]).toMatchObject({ + currentParams: {}, + currentUrl: new URL("http://localhost/"), + nextParams: {}, + nextUrl: new URL("http://localhost/child"), + defaultShouldRevalidate: false, + }); + expect(router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "FETCH 1", + }); + router.navigate("/"); await tick(); - expect(shouldRevalidate.mock.calls.length).toBe(0); + expect(shouldRevalidate.mock.calls.length).toBe(2); + expect(shouldRevalidate.mock.calls[1][0]).toMatchObject({ + currentParams: {}, + currentUrl: new URL("http://localhost/child"), + nextParams: {}, + nextUrl: new URL("http://localhost/"), + defaultShouldRevalidate: false, + }); expect(router.state.fetchers.get(key)).toMatchObject({ state: "idle", data: "FETCH 1", }); - expect(shouldRevalidate.mock.calls.length).toBe(0); - // Post navigation should trigger shouldRevalidate, and loader should not re-run + // Submission navigations should trigger fetcher shouldRevalidate with + // defaultShouldRevalidate=true router.navigate("/child", { formMethod: "post", formData: createFormData({}), @@ -1979,12 +1992,12 @@ describe("a router", () => { state: "idle", data: "FETCH 1", }); - expect(shouldRevalidate.mock.calls.length).toBe(1); - expect(shouldRevalidate.mock.calls[0][0]).toMatchObject({ + expect(shouldRevalidate.mock.calls.length).toBe(3); + expect(shouldRevalidate.mock.calls[2][0]).toMatchObject({ currentParams: {}, - currentUrl: new URL("http://localhost/fetch"), + currentUrl: new URL("http://localhost/"), nextParams: {}, - nextUrl: new URL("http://localhost/fetch"), + nextUrl: new URL("http://localhost/child"), formAction: "/child", formData: createFormData({}), formEncType: "application/x-www-form-urlencoded", @@ -5923,9 +5936,19 @@ describe("a router", () => { it("supports relative routing in redirects (from parent fetch loader)", async () => { let t = setup({ routes: REDIRECT_ROUTES }); - let fetch = await t.fetch("/parent"); + let fetch = await t.fetch("/parent", "key"); + + let B = await fetch.loaders.parent.redirectReturn( + "..", + undefined, + undefined, + ["parent"] + ); - await fetch.loaders.parent.redirectReturn(".."); + // We called fetcher.load('/parent') from the root route, so when we + // redirect back to the root it triggers a revalidation of the + // fetcher.load('/parent') + await B.loaders.parent.resolve("Revalidated"); // No root loader so redirect lands immediately expect(t.router.state).toMatchObject({ location: { @@ -5935,6 +5958,10 @@ describe("a router", () => { loaderData: {}, errors: null, }); + expect(t.router.state.fetchers.get("key")).toMatchObject({ + state: "idle", + data: "Revalidated", + }); }); it("supports relative routing in redirects (from child fetch loader)", async () => { @@ -9117,6 +9144,74 @@ describe("a router", () => { }); }); + it("revalidates fetchers on searchParams changes", async () => { + let key = "key"; + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/tasks/1"], + hydrationData: { + loaderData: { + root: "ROOT", + taskId: "TASK 1", + }, + }, + }); + + let A = await t.fetch("/?index", key); + await A.loaders.index.resolve("FETCH 1"); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "FETCH 1", + }); + + let B = await t.navigate("/tasks/1?key=value", undefined, ["index"]); + await B.loaders.root.resolve("ROOT 2"); + await B.loaders.tasksId.resolve("TASK 2"); + await B.loaders.index.resolve("FETCH 2"); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT 2", + tasksId: "TASK 2", + }); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "FETCH 2", + }); + }); + + it("revalidates fetchers on links to the current location", async () => { + let key = "key"; + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/tasks/1"], + hydrationData: { + loaderData: { + root: "ROOT", + taskId: "TASK 1", + }, + }, + }); + + let A = await t.fetch("/?index", key); + await A.loaders.index.resolve("FETCH 1"); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "FETCH 1", + }); + + let B = await t.navigate("/tasks/1", undefined, ["index"]); + await B.loaders.root.resolve("ROOT 2"); + await B.loaders.tasksId.resolve("TASK 2"); + await B.loaders.index.resolve("FETCH 2"); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT 2", + tasksId: "TASK 2", + }); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "FETCH 2", + }); + }); + it("does not revalidate idle fetchers when a loader navigation is performed", async () => { let key = "key"; let t = setup({ @@ -9149,13 +9244,23 @@ describe("a router", () => { let count = 0; let shouldRevalidate = jest.fn((args) => false); let router = createRouter({ - history: createMemoryHistory({ initialEntries: ["/"] }), + history: createMemoryHistory({ initialEntries: ["/one"] }), routes: [ { id: "root", path: "/", loader: () => Promise.resolve(++count), - action: () => Promise.resolve(null), + children: [ + { + path: ":a", + children: [ + { + path: ":b", + action: () => Promise.resolve(null), + }, + ], + }, + ], }, { id: "fetch", @@ -9183,7 +9288,7 @@ describe("a router", () => { }); // Post to the current route - router.navigate("/", { + router.navigate("/two/three", { formMethod: "post", formData: createFormData({}), }); @@ -9198,15 +9303,20 @@ describe("a router", () => { expect(shouldRevalidate.mock.calls[0][0]).toMatchInlineSnapshot(` { "actionResult": null, - "currentParams": {}, - "currentUrl": "http://localhost/fetch", + "currentParams": { + "a": "one", + }, + "currentUrl": "http://localhost/one", "defaultShouldRevalidate": true, - "formAction": "/", + "formAction": "/two/three", "formData": FormData {}, "formEncType": "application/x-www-form-urlencoded", "formMethod": "post", - "nextParams": {}, - "nextUrl": "http://localhost/fetch", + "nextParams": { + "a": "two", + "b": "three", + }, + "nextUrl": "http://localhost/two/three", } `); @@ -9323,41 +9433,92 @@ describe("a router", () => { }); }); + it("does not revalidate fetchers initiated from removed routes", async () => { + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { loaderData: { root: "ROOT", index: "INDEX" } }, + }); + + let key = "key"; + + // Trigger a fetch from the index route + let A = await t.fetch("/tasks/1", key, "index"); + await A.loaders.tasksId.resolve("TASKS"); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "TASKS", + }); + + // Navigate such that the index route will be removed + let B = await t.navigate("/tasks", { + formMethod: "post", + formData: createFormData({}), + }); + + // Resolve the action + await B.actions.tasks.resolve("TASKS ACTION"); + + // Fetcher should remain in an idle state since it's calling route is + // being removed + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "TASKS", + }); + + // Resolve navigation loaders + await B.loaders.root.resolve("ROOT*"); + await B.loaders.tasks.resolve("TASKS LOADER"); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.location.pathname).toBe("/tasks"); + + // Fetcher never got called + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "TASKS", + }); + }); + it("cancels in-flight fetcher.loads on action submission and forces reload", async () => { let t = setup({ routes: [ { - id: "index", - index: true, - }, - { - id: "action", - path: "action", - action: true, - }, - // fetch A will resolve before the action and will be able to opt-out - { - id: "fetchA", - path: "fetch-a", - loader: true, - shouldRevalidate: () => false, - }, - // fetch B will resolve before the action but then issue a second - // load that gets cancelled. It will not be able to opt out because - // of the cancellation - { - id: "fetchB", - path: "fetch-b", - loader: true, - shouldRevalidate: () => false, - }, - // fetch C will not before the action, and will not be able to opt - // out because it has no data - { - id: "fetchC", - path: "fetch-c", - loader: true, - shouldRevalidate: () => false, + path: "/", + children: [ + { + id: "index", + index: true, + }, + { + id: "action", + path: "action", + action: true, + }, + // fetch A will resolve before the action and will be able to opt-out + { + id: "fetchA", + path: "fetch-a", + loader: true, + shouldRevalidate: () => false, + }, + // fetch B will resolve before the action but then issue a second + // load that gets cancelled. It will not be able to opt out because + // of the cancellation + { + id: "fetchB", + path: "fetch-b", + loader: true, + shouldRevalidate: () => false, + }, + // fetch C will not before the action, and will not be able to opt + // out because it has no data + { + id: "fetchC", + path: "fetch-c", + loader: true, + shouldRevalidate: () => false, + }, + ], }, ], initialEntries: ["/"], diff --git a/packages/router/router.ts b/packages/router/router.ts index 6b40d6bb10..e4e30beff1 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -21,6 +21,7 @@ import type { SuccessResult, AgnosticRouteMatch, MutationFormMethod, + ShouldRevalidateFunction, } from "./utils"; import { DeferredData, @@ -549,25 +550,22 @@ interface HandleLoadersResult extends ShortCircuitable { } /** - * Tuple of [key, href, DataRouteMatch, DataRouteMatch[]] for a revalidating - * fetcher.load() + * Cached info for active fetcher.load() instances so they can participate + * in revalidation */ -type RevalidatingFetcher = [ - string, - string, - AgnosticDataRouteMatch, - AgnosticDataRouteMatch[] -]; +interface FetchLoadMatch { + routeId: string; + path: string; + match: AgnosticDataRouteMatch; + matches: AgnosticDataRouteMatch[]; +} /** - * Tuple of [href, DataRouteMatch, DataRouteMatch[]] for an active - * fetcher.load() + * Identified fetcher.load() calls that need to be revalidated */ -type FetchLoadMatch = [ - string, - AgnosticDataRouteMatch, - AgnosticDataRouteMatch[] -]; +interface RevalidatingFetcher extends FetchLoadMatch { + key: string; +} /** * Wrapper object to allow us to throw any response out from callLoaderOrAction @@ -1380,8 +1378,8 @@ export function createRouter(init: RouterInit): Router { // preserving any new action data or existing action data (in the case of // a revalidation interrupting an actionReload) if (!isUninterruptedRevalidation) { - revalidatingFetchers.forEach(([key]) => { - let fetcher = state.fetchers.get(key); + revalidatingFetchers.forEach((rf) => { + let fetcher = state.fetchers.get(rf.key); let revalidatingFetcher: FetcherStates["Loading"] = { state: "loading", data: fetcher && fetcher.data, @@ -1391,7 +1389,7 @@ export function createRouter(init: RouterInit): Router { formData: undefined, " _hasFetcherDoneAnything ": true, }; - state.fetchers.set(key, revalidatingFetcher); + state.fetchers.set(rf.key, revalidatingFetcher); }); let actionData = pendingActionData || state.actionData; updateState({ @@ -1408,8 +1406,8 @@ export function createRouter(init: RouterInit): Router { } pendingNavigationLoadId = ++incrementingLoadId; - revalidatingFetchers.forEach(([key]) => - fetchControllers.set(key, pendingNavigationController!) + revalidatingFetchers.forEach((rf) => + fetchControllers.set(rf.key, pendingNavigationController!) ); let { results, loaderResults, fetcherResults } = @@ -1428,7 +1426,7 @@ export function createRouter(init: RouterInit): Router { // Clean up _after_ loaders have completed. Don't clean up if we short // circuited because fetchControllers would have been aborted and // reassigned to new controllers for the next navigation - revalidatingFetchers.forEach(([key]) => fetchControllers.delete(key)); + revalidatingFetchers.forEach((rf) => fetchControllers.delete(rf.key)); // If any loaders returned a redirect Response, start a new REPLACE navigation let redirect = findRedirect(results); @@ -1514,7 +1512,7 @@ export function createRouter(init: RouterInit): Router { // Store off the match so we can call it's shouldRevalidate on subsequent // revalidations - fetchLoadMatches.set(key, [path, match, matches]); + fetchLoadMatches.set(key, { routeId, path, match, matches }); handleFetcherLoader(key, routeId, path, match, matches, submission); } @@ -1651,8 +1649,9 @@ export function createRouter(init: RouterInit): Router { // current fetcher which we want to keep in it's current loading state which // contains it's action submission info + action data revalidatingFetchers - .filter(([staleKey]) => staleKey !== key) - .forEach(([staleKey]) => { + .filter((rf) => rf.key !== key) + .forEach((rf) => { + let staleKey = rf.key; let existingFetcher = state.fetchers.get(staleKey); let revalidatingFetcher: FetcherStates["Loading"] = { state: "loading", @@ -1684,9 +1683,7 @@ export function createRouter(init: RouterInit): Router { fetchReloadIds.delete(key); fetchControllers.delete(key); - revalidatingFetchers.forEach(([staleKey]) => - fetchControllers.delete(staleKey) - ); + revalidatingFetchers.forEach((r) => fetchControllers.delete(r.key)); let redirect = findRedirect(results); if (redirect) { @@ -1980,12 +1977,12 @@ export function createRouter(init: RouterInit): Router { ...matchesToLoad.map((match) => callLoaderOrAction("loader", request, match, matches, router.basename) ), - ...fetchersToLoad.map(([, href, match, fetchMatches]) => + ...fetchersToLoad.map((f) => callLoaderOrAction( "loader", - createClientSideRequest(init.history, href, request.signal), - match, - fetchMatches, + createClientSideRequest(init.history, f.path, request.signal), + f.match, + f.matches, router.basename ) ), @@ -2004,7 +2001,7 @@ export function createRouter(init: RouterInit): Router { ), resolveDeferredResults( currentMatches, - fetchersToLoad.map(([, , match]) => match), + fetchersToLoad.map((f) => f.match), fetcherResults, request.signal, true @@ -2892,47 +2889,81 @@ function getMatchesToLoad( ? Object.values(pendingActionData)[0] : undefined; + let currentUrl = history.createURL(state.location); + let nextUrl = history.createURL(location); + + let defaultShouldRevalidate = + // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate + isRevalidationRequired || + // Clicked the same link, resubmitted a GET form + currentUrl.toString() === nextUrl.toString() || + // Search params affect all loaders + currentUrl.search !== nextUrl.search; + // Pick navigation matches that are net-new or qualify for revalidation let boundaryId = pendingError ? Object.keys(pendingError)[0] : undefined; let boundaryMatches = getLoaderMatchesUntilBoundary(matches, boundaryId); - let navigationMatches = boundaryMatches.filter( - (match, index) => - match.route.loader != null && - (isNewLoader(state.loaderData, state.matches[index], match) || - // If this route had a pending deferred cancelled it must be revalidated - cancelledDeferredRoutes.some((id) => id === match.route.id) || - shouldRevalidateLoader( - history, - state.location, - state.matches[index], - submission, - location, - match, - isRevalidationRequired, - actionResult - )) - ); + + let navigationMatches = boundaryMatches.filter((match, index) => { + if (match.route.loader == null) { + return false; + } + + // Always call the loader on new route instances and pending defer cancellations + if ( + isNewLoader(state.loaderData, state.matches[index], match) || + cancelledDeferredRoutes.some((id) => id === match.route.id) + ) { + return true; + } + + // This is the default implementation for when we revalidate. If the route + // provides it's own implementation, then we give them full control but + // provide this value so they can leverage it if needed after they check + // their own specific use cases + let currentRouteMatch = state.matches[index]; + let nextRouteMatch = match; + + return shouldRevalidateLoader(match, { + currentUrl, + currentParams: currentRouteMatch.params, + nextUrl, + nextParams: nextRouteMatch.params, + ...submission, + actionResult, + defaultShouldRevalidate: + defaultShouldRevalidate || + isNewRouteInstance(currentRouteMatch, nextRouteMatch), + }); + }); // Pick fetcher.loads that need to be revalidated let revalidatingFetchers: RevalidatingFetcher[] = []; fetchLoadMatches && - 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, fetchMatches]); - } else if (isRevalidationRequired) { - let shouldRevalidate = shouldRevalidateLoader( - history, - href, - match, - submission, - href, - match, - isRevalidationRequired, - actionResult - ); + fetchLoadMatches.forEach((f, key) => { + if (!matches.some((m) => m.route.id === f.routeId)) { + // This fetcher is not going to be present in the subsequent render so + // there's no need to revalidate it + return; + } else if (cancelledFetcherLoads.includes(key)) { + // This fetcher was cancelled from a prior action submission - force reload + revalidatingFetchers.push({ key, ...f }); + } else { + // Revalidating fetchers are decoupled from the route matches since they + // hit a static href, so they _always_ check shouldRevalidate and the + // default is strictly if a revalidation is explicitly required (action + // submissions, useRevalidator, X-Remix-Revalidate). + let shouldRevalidate = shouldRevalidateLoader(f.match, { + currentUrl, + currentParams: state.matches[state.matches.length - 1].params, + nextUrl, + nextParams: matches[matches.length - 1].params, + ...submission, + actionResult, + defaultShouldRevalidate, + }); if (shouldRevalidate) { - revalidatingFetchers.push([key, href, match, fetchMatches]); + revalidatingFetchers.push({ key, ...f }); } } }); @@ -2969,58 +3000,24 @@ function isNewRouteInstance( currentMatch.pathname !== match.pathname || // splat param changed, which is not present in match.path // e.g. /files/images/avatar.jpg -> files/finances.xls - (currentPath && + (currentPath != null && currentPath.endsWith("*") && currentMatch.params["*"] !== match.params["*"]) ); } function shouldRevalidateLoader( - history: History, - currentLocation: string | Location, - currentMatch: AgnosticDataRouteMatch, - submission: Submission | undefined, - location: string | Location, - match: AgnosticDataRouteMatch, - isRevalidationRequired: boolean, - actionResult: DataResult | undefined + loaderMatch: AgnosticDataRouteMatch, + arg: Parameters[0] ) { - let currentUrl = history.createURL(currentLocation); - let currentParams = currentMatch.params; - let nextUrl = history.createURL(location); - let nextParams = match.params; - - // This is the default implementation as to when we revalidate. If the route - // provides it's own implementation, then we give them full control but - // provide this value so they can leverage it if needed after they check - // their own specific use cases - // Note that fetchers always provide the same current/next locations so the - // URL-based checks here don't apply to fetcher shouldRevalidate calls - let defaultShouldRevalidate = - isNewRouteInstance(currentMatch, match) || - // Clicked the same link, resubmitted a GET form - currentUrl.toString() === nextUrl.toString() || - // Search params affect all loaders - currentUrl.search !== nextUrl.search || - // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate - isRevalidationRequired; - - if (match.route.shouldRevalidate) { - let routeChoice = match.route.shouldRevalidate({ - currentUrl, - currentParams, - nextUrl, - nextParams, - ...submission, - actionResult, - defaultShouldRevalidate, - }); + if (loaderMatch.route.shouldRevalidate) { + let routeChoice = loaderMatch.route.shouldRevalidate(arg); if (typeof routeChoice === "boolean") { return routeChoice; } } - return defaultShouldRevalidate; + return arg.defaultShouldRevalidate; } async function callLoaderOrAction( @@ -3340,7 +3337,7 @@ function processLoaderData( // Process results from our revalidating fetchers for (let index = 0; index < revalidatingFetchers.length; index++) { - let [key, , match] = revalidatingFetchers[index]; + let { key, match } = revalidatingFetchers[index]; invariant( fetcherResults !== undefined && fetcherResults[index] !== undefined, "Did not find corresponding fetcher result"