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

Avoid calling shouldRevalidate on interrupted initial load fetchers #10623

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

brophdawg11
Copy link
Contributor

If an initial fetcher.load is interrupted by a navigation, we should not try to call shouldRevalidate for the interrupted fetcher since it still performing the initial load and therefore it's not a revalidation. This is specifically crucial in Remix apps where the "load" also encompasses loading the JS route module, so until the first one completes we don't even have a shouldRevalidate implementation to call.

Also ensures we properly abort in-progress fetcher.load calls that are re-loaded in this manner.

Closes #10473

@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2023

🦋 Changeset detected

Latest commit: 0491082

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

@brophdawg11 brophdawg11 linked an issue Jun 20, 2023 that may be closed by this pull request
Comment on lines +1543 to +1545
if (fetchControllers.has(rf.key)) {
abortFetcher(rf.key);
}
Copy link
Contributor Author

@brophdawg11 brophdawg11 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we're about to reload/revalidate a fetcher.load call, ensure we abort any in-progress loads before we overwrite them with a new controller

Comment on lines +3384 to +3390
let isPerformingInitialLoad =
fetcher &&
fetcher.state !== "idle" &&
fetcher.data === undefined &&
// If a fetcher.load redirected then it'll be "loading" without any data
// so ensure we're not processing the redirect from this fetcher
!fetchRedirectIds.has(key);
Copy link
Contributor Author

@brophdawg11 brophdawg11 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the net new condition, otherwise this code just combines the if blocks for cancelledFetcherLoads/isPerformingInitialLoad/shouldRevalidate since they all populate the same object into revalidatingFetchers.

@brophdawg11 brophdawg11 merged commit 718ec1f into dev Jun 21, 2023
@brophdawg11 brophdawg11 deleted the brophdawg11/fetcher-shouldrevalidate-race branch June 21, 2023 14:02
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.14.0-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.14.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.

[Bug]: Fetcher Revalidation Race Condition (Remix)
2 participants