Rework infinite query forced checks #4854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c9d6a4e:
|
size-limit report 📦
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
markerikson
merged commit 8a4ff3a
into
feature/infinite-query-integration
88 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously in #4795 , I had made tweaks to the infinite query fetching logic to fix broken behavior (would not fetch next pages if the API definition had
refetchOnMountOrArgChange: true). I did so by altering the logic to fall back to an empty infinite data set, using an internalarg.forceRefetchflag instead ofisForcedQuery().@agusterodin reported some more problems with similar scenarios in #4744 (comment). I did some investigating and concluded that this change resulted in a logic mismatch. We triggered an actual run of the thunk because
conditionusedisForcedQuery, but we didn't useisForcedQueryto determine whether to use cached data or start fresh. That led to a refetch of the pages, but appended to the existing copy of the pages, resulting in duplicates.After further tinkering, I ended up with
isForcedQuery() && !arg.direction, as we don't pass in adirectionflag when the hook switches to a different arg. That appears to handle both "can fetch withrefetchOnMountOrArgChangeactive" and "does not duplicate the entries on refetch".As for the other reported issue, where an arg change + refetch is quickly followed by a
fetchNextPage()call: it looks like React Query intentionally cancels an in-flight query in that case. It seems like the refetch sequence continues, but I think that promise gets ignored. So, the additional page request goes out right away.It might be possible to mimic that behavior with RTKQ, but our internal logic is different enough it's a bit trickier. Given that, I'm going to skip trying to match that edge case for now.