Fix issue where a merge function that returns an incomplete result would not allow the client to refetch from network #11839
Conversation
🦋 Changeset detectedLatest commit: a4750ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
|
/release:pr |
|
A new release has been made for this PR. You can install it with: |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -4281,6 +4281,543 @@ describe("useQuery Hook", () => { | |||
| await expect(Profiler).not.toRerender(); | |||
| }); | |||
|
|
|||
| it("rerenders errored query for full cache write", async () => { | |||
There was a problem hiding this comment.
This is mainly a "documenting" test case since this behavior is currently built-in. I verified that we can write a full cache result to a query that had returned an error and have it re-render with the full cache result.
This may or may not be something we want to revisit in a future major since this has implications on UX, especially for overlapping queries.
| await expect(Profiler).not.toRerender(); | ||
| }); | ||
|
|
||
| it("delivers the full network response when a merge function returns an incomplete result", async () => { |
There was a problem hiding this comment.
This is also a documenting test. I wanted to make sure we captured this as behavior in the client since I did not expect to see this behavior work.
| // go and fetch the missing data. | ||
| !(oldDiff && oldDiff.complete) | ||
| ) { | ||
| if (diff && !diff.complete && this.observableQuery?.getLastError()) { |
There was a problem hiding this comment.
Wouldn't an update be okay in an error-but returnPartialData: true case?
Staying closer to the previous logic (although I'll admit I'm a bit hazy on some of it)
if (
diff &&
!diff.complete &&
+ this.observableQuery?.getLastError() &&
!this.observableQuery?.options.returnPartialData &&
// In the case of a cache eviction, the diff will become partial so we
// schedule a notification to send a network request (this.oqListener) to
// go and fetch the missing data.
!(oldDiff && oldDiff.complete)
) {
|
There's no perfect behaviour here, but this is probably less surprising. |
Fixes #11759
Modifies the logic added in #11579 which fixed an issue where a partial cache update caused it to be reported to a query that had an error. There were some situations in which a partial cache write should cause a refetch from the server in order to fulfill the data requirements in the query. The check from #11579 did not allow this to perform correctly as it assumed that all partial cache updates were "bad" and did not report them.
This change tweaks the check to only swallow the partial update if the query has previously errored, which maintains the change that #11579 was intending to fix. This reverts back to more closely resemble the logic from 3.9.4 which allowed the partial cache result to cause a network request.