-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix fetcher shouldRevalidate parameters #9948
Conversation
🦋 Changeset detectedLatest commit: d4b1b18 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 |
// Normal navigations should trigger fetcher shouldRevalidate with | ||
// defaultShouldRevalidate=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test used to incorrectly assert that we did not even call shouldRevalidate
. But instead, on normal navigations we should be calling shouldRevalidate
on fetcher routes if it exists, and giving it a defaultShouldRevalidate:false
value since it's just a normal navigation. This aligns with our goal of giving the user op-in and opt-out control
currentUrl: new URL("http://localhost/fetch"), | ||
currentUrl: new URL("http://localhost/"), | ||
nextParams: {}, | ||
nextUrl: new URL("http://localhost/fetch"), | ||
nextUrl: new URL("http://localhost/child"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These now align with the navigation urls, not the identical fetcher.load
url for both
it("cancels in-flight fetcher.loads on action submission and forces reload", async () => { | ||
let t = setup({ | ||
routes: [ | ||
{ | ||
id: "index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapped all these in a parent route so the fetchers don't get skipped due to removed routes :)
packages/router/router.ts
Outdated
@@ -1514,7 +1516,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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Track the calling routeId
so we can skip fetcher.load
revalidations on fetchers that are about to be deleted
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared amongst navigation and fetcher shouldRevalidate
calls now
actionResult, | ||
defaultShouldRevalidate: | ||
defaultShouldRevalidate || | ||
isNewRouteInstance(currentRouteMatch, nextRouteMatch), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Navigations also look for param changes on the individual route match level as part of defaultShouldRevalidate
path: string; | ||
match: AgnosticDataRouteMatch; | ||
matches: AgnosticDataRouteMatch[]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed these to object types for clarity
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
This PR fixes underlying some bugs with
fetcher.load
shouldRevalidate
behavior. In an oversight in initial implementation we passed the fetcher.load() url to both thecurrentUrl
/nextUrl
parameters and also deciphered thecurrentParams
/nextParams
from the fetcher load match, not the navigation locations. This is wrong since fetchers revalidate due to navigation events and we need to provide information about the navigation that triggered the revalidation (and indeed theform*
submission fields we provide come from the navigation).This PR makes the following updates:
shouldRevalidate
now receivescurrentUrl
/currentParams
/nextUrl
/nextParams
fields from the navigation locations to align with the submissions infoCloses remix-run/remix#5090