-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 an issue where incorrect results are returned when changing variables and skipping #12461
Conversation
🦋 Changeset detectedLatest commit: fba44bf 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 |
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: ebdd4ed547fdaec639db5627 |
commit: |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
variables: { id: 1 }, | ||
}); | ||
|
||
await rerender({ id: 2, skip: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the core issues at heart. When variables change and skipping a query (which sets the fetchPolicy
to standby
), getDiff
is not called, which meant that updateWatch
in QueryInfo
was not called with the new variables. If a cache update occurred while the query was in standby (line 1697 in this test), the lastDiff
was updated with the result from old variables. QueryInfo.getDiff()
then just returned that lastDiff
from that point forward because it didn't think variables had changed.
|
||
using _disabledAct = disableActEnvironment(); | ||
const { takeSnapshot, rerender } = await renderHookToSnapshotStream( | ||
({ id, skip }) => useQuery(query, { skip, variables: { id } }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to whittle this reproduction down to skip
and variables
without the need for another query to reproduce. The trick is writing to change variables and set skip
at the same time, then issue cache updates while the query is in standby to reproduce this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, that one looks like it was tricky to find - good call!
Fixes #12458
If a query changed variables and simultaneously skipped a query,
QueryInfo
would get updated with the rightvariables
, but the previouscache.watch
was active. This was a problem when thefetchPolicy
changed tostandby
as a result of skipping the query while changing variables. If something else cache a cache update during the period while the query was in standby, the diff was updated to the result from the previous variables which meant the next timequeryInfo.getDiff()
was called, it used the already stored wrong result.This change cancels a
cache.watch
if variables change so that cache updates while a query is skipped don't updateQueryInfo
.