Fix regression that causes partial data to be reported unexpectedly in some circumstances #11579
Conversation
🦋 Changeset detectedLatest commit: 1e408bd 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 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jerelmiller
changed the title
jerelmiller
changed the title
src/core/ObservableQuery.ts
Outdated
| // QueryInfo calls cache.watch with returnPartialData set to `true` always, | ||
| // which means that cache broadcasts for some queries will store the | ||
| // lastResult with partial data even though we don't tolerate it. | ||
| if (result.partial && !this.options.returnPartialData) { | ||
| result.data = void 0 as any; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd love it if this were the solution, but this will reset deferred results in the second call to getCurrentResult.
Here is a failing test:
it.only("should handle multiple calls to getCurrentResult without losing data", async () => {
const query = gql`
{
greeting {
message
... on Greeting @defer {
recipient {
name
}
}
}
}
`;
const link = new MockSubscriptionLink();
const client = new ApolloClient({
link,
cache: new InMemoryCache(),
});
const obs = client.watchQuery({ query });
const stream = new ObservableStream(obs);
setTimeout(() => {
link.simulateResult({
result: {
data: {
greeting: {
message: "Hello world",
__typename: "Greeting",
},
},
hasNext: true,
},
});
});
{
const result = await stream.takeNext();
expect(result.data).toEqual({
greeting: {
message: "Hello world",
__typename: "Greeting",
},
});
expect(obs.getCurrentResult().data).toEqual({
greeting: {
message: "Hello world",
__typename: "Greeting",
},
});
// second call to `getCurrentResult`
expect(obs.getCurrentResult().data).toEqual({
greeting: {
message: "Hello world",
__typename: "Greeting",
},
});
}
});There was a problem hiding this comment.
Thank you. I was going to dig more into possible fail scenarios today since @defer uses partial. Thanks for accelerating my findings 🙂
There was a problem hiding this comment.
We ran into this issue after updating the package, and releasing it yesterday. Thank you for working on this fix!
jerelmiller
force-pushed
the
jerel/issue-11400-unexpected-partial-data
branch
from
b169071
to
5b1853d
Compare
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
| }); | ||
| } | ||
|
|
||
| await expect(Profiler).not.toRerender(); |
There was a problem hiding this comment.
Yes, I think it should be mentioned in the changelog.
Maybe add a second changeset to the PR and also classify it as a bug-fix/patch. It's probably better to have as a separate point.
|
@phryneas this should be good to go. I've addressed all the comments and am happy with the diff in this PR. Appreciate the suggestions! |
|
If during handling a response is found an error (for example a missed field described inside "typePolicies" for cache), when "useQuery" won't be triggered after the mutation gets the update. The same code that works in 3.9.4 after the update to 3.9.5 won't work. FYI @jerelmiller |
|
@av-k do you have a code sample on what you mean? To be honest, I'm having a bit of a difficult time visualizing what you're saying. Are you perhaps experiencing the thing mentioned at the bottom of the PR description?
|
|
@jerelmiller PR description? - it's not about the mentioned problem. In order to provide a code sample, I will require a demo GraphQL server with a schema containing nested objects within an entity. So, to be able to reproduce this issue just get an error during handling the response, so here: So if there is an error (for example the incorrect configuration of "typePolicies") - "diff.complete" will be always "false". |
|
@av-k Koennten Sie das Problem vielleicht noch einmal etwas ausfuehrlicher auf Deutsch beschreiben? |
|
@phryneas Thank you for the suggestion, but I will still try :) Unfortunately, I was unable to achieve an error regarding the missing field using the codesandbox example, so I reproduced a simpler case with a field merge error in "typePolicies" (index.js:22). I'm attaching two identical codesandboxes, the only difference being the versions of @apollo/client. @apollo/client v.3.9.5 @apollo/client v.3.9.4 Steps to reproduce:
Actual behavior (in version 3.9.5 - broken): Expected behavior (in version 3.9.4 - works): P.S. At first glance, it may seem strange why I presented a code example where I intentionally broke cache management functionality. There are two main concerns on my part:
|
|
@jerelmiller is there any updates? |
Fixes #11400
Fixes a regression introduced by #8642 in 2d5e498 that causes partial data to be reported by
useQuery/useLazyQuerywhen one query fails and another succeeds with overlapping data. This is specifically triggered whennotifyOnNetworkStatusis set totrueon the first failing query. The cache write from the second query triggers the first query to try and refetch since there is missing field data in that query. BecausenotifyOnNetworkStatuswas set totrue, there was a render that contained additional partial data since this was set on thelastResultby the cache broadcast.This PR adjusts the
QueryInfo.setDifffunction to ignore cache results that contain partial data whenreturnPartialDatais set tofalse.Important
Previously queries with errors would auto refetch when a partial cache update was written that overlapped with the query. This is no longer the case since partial cache updates are ignored and not reported.