Fix fetcher revalidation logic #10344
Conversation
brophdawg11
commented
Apr 14, 2023
brophdawg11
commented
- Fetchers no longer revalidate on search param changes or when routing to the same URL
- Also fixed a semi-related bug where fetcher.load redirects wouldn't be marked as done on the completion of the redirect
🦋 Changeset detectedLatest commit: 53698a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // 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"); |
There was a problem hiding this comment.
This test flow no longer triggers a fetcher revalidation
| @@ -814,7 +814,7 @@ export function createRouter(init: RouterInit): Router { | |||
| // Fetchers that triggered data reloads as a result of their actions | |||
| let fetchReloadIds = new Map<string, number>(); | |||
|
|
|||
| // Fetchers that triggered redirect navigations from their actions | |||
| // Fetchers that triggered redirect navigations | |||
| let fetchRedirectIds = new Set<string>(); | |||
There was a problem hiding this comment.
This captures fetcher.load redirects now as well
| @@ -1470,12 +1470,14 @@ export function createRouter(init: RouterInit): Router { | |||
|
|
|||
| // Short circuit if we have no loaders to run | |||
| if (matchesToLoad.length === 0 && revalidatingFetchers.length === 0) { | |||
| let updatedFetchers = markFetchRedirectsDone(); | |||
There was a problem hiding this comment.
This was a bug before as well - but since fetcher's were too-eagerly revalidating it wasn't common to have a fetch-driven redirect that triggered zero loaders.
| ...(didAbortFetchLoads || revalidatingFetchers.length > 0 | ||
| ? { fetchers: new Map(state.fetchers) } | ||
| : {}), | ||
| ...(shouldUpdateFetchers ? { fetchers: new Map(state.fetchers) } : {}), |
There was a problem hiding this comment.
Ensure that if any fetch redirects got cleaned up we commit them
| @@ -1971,6 +1973,7 @@ export function createRouter(init: RouterInit): Router { | |||
|
|
|||
| // If the loader threw a redirect Response, start a new REPLACE navigation | |||
| if (isRedirectResult(result)) { | |||
| fetchRedirectIds.add(key); | |||
There was a problem hiding this comment.
Track fetcher.load redirects as well so they can get cleaned up on completion of the redirect navigation
- Fetchers no longer revalidate on search param changes or when routing to the same URL - Also fixed a semi-related bug where fetcher.load redirects wouldn't be marked as done on the completion of the redirect
brophdawg11
force-pushed
the
brophdawg11/fetcher-revalidation
branch
from
99b3acc
to
7603a77
Compare
| @@ -9578,7 +9571,7 @@ describe("a router", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it("revalidates fetchers on searchParams changes", async () => { | |||
| it("does not revalidate fetchers on searchParams changes", async () => { | |||
There was a problem hiding this comment.
Change tests asserting the old incorrect behavior
| @@ -3231,7 +3234,8 @@ function getMatchesToLoad( | |||
| nextParams: matches[matches.length - 1].params, | |||
| ...submission, | |||
| actionResult, | |||
| defaultShouldRevalidate, | |||
| // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate | |||
| defaultShouldRevalidate: isRevalidationRequired, | |||
There was a problem hiding this comment.
This is the crux of the bug fix. Fetchers should not use the same logic as routes (which takes into account search params and same-URL navigations). They should only revalidate on action submissions, router.revalidate, or X-Remix-Revalidate
|
🤖 Hello there, We just published version Thanks! |
|
🤖 Hello there, We just published version Thanks! |
