Skip to content
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

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Jan 19, 2023

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 the currentUrl/nextUrl parameters and also deciphered the currentParams/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 the form* submission fields we provide come from the navigation).

This PR makes the following updates:

  • fetcher shouldRevalidate now receives currentUrl/currentParams/nextUrl/nextParams fields from the navigation locations to align with the submissions info
  • I also realized that we are revalidating all fetchers - but since we know the routeId that triggered a fetcher.load, we can skip revalidation on fetcher.load calls coming from routes we are removing as part of the current navigation

Closes remix-run/remix#5090

@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2023

🦋 Changeset detected

Latest commit: d4b1b18

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Patch
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

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

Comment on lines +1953 to +1954
// Normal navigations should trigger fetcher shouldRevalidate with
// defaultShouldRevalidate=false
Copy link
Contributor Author

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

Comment on lines -1985 to +2001
currentUrl: new URL("http://localhost/fetch"),
currentUrl: new URL("http://localhost/"),
nextParams: {},
nextUrl: new URL("http://localhost/fetch"),
nextUrl: new URL("http://localhost/child"),
Copy link
Contributor Author

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",
Copy link
Contributor Author

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 :)

@@ -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]);
Copy link
Contributor Author

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

Comment on lines +2897 to +2906
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;
Copy link
Contributor Author

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),
Copy link
Contributor Author

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[];
}
Copy link
Contributor Author

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

@brophdawg11 brophdawg11 merged commit 03da9d6 into dev Jan 24, 2023
@brophdawg11 brophdawg11 deleted the brophdawg11/fetcher-shouldRevalidate branch January 24, 2023 21:14
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.7.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.8.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useFetcher: fetcher.load doesn't revalidate when search params change
4 participants