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 voiding result.data in ObservableQuery#getCurrentResult when !diff.complete #8642

Merged
merged 10 commits into from
Aug 16, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 12, 2021

PR #8422 (released in @apollo/client@3.4.0-rc.15) attempted to solve issue #7978 (reported by @dylanwulf) by relying on queryInfo.getDiff() to obtain results appropriate for the current variables, rather than relying on the lastResult stored by the ObservableQuery (which may have been obtained using different variables).

While this solution appeared to resolve #7978, it introduced a new bug (reported in #8620 by @maxsalven) because the following code now runs unconditionally:

result.data = (
diff.complete ||
this.options.returnPartialData
) ? diff.result : void 0;

Whenever !(diff.complete || this.options.returnPartialData), this code clobbers result.data with void 0, even though result.data might provide useful information. In the case of #8620, that clobbered information is the missing network result for the A query.

Ideally, we should leave result.data unmodified except when diff.result is used, as follows:

if (diff.complete || this.options.returnPartialData) {
result.data = diff.result;
}

This change helps with issue #8620, but introduces some new test failures, because some of the result.data objects that are now delivered (instead of void 0) were obtained using different/stale variables than the current this.options.variables. In other words, these test failures can be prevented if we finish fixing #7978, because the previous result will be appropriately ignored by getCurrentResult when the variables have changed.

The rest of the commits in this PR fix those tests by tracking a snapshot of the original variables associated with the last result, so the getCurrentResult method can avoid using this.last.result if this.last.variables no longer match this.options.variables.

I also considered reverting PR #8422 and building back up to a better solution for #7978, but #8422 wasn't all bad, and I was able to make everything work more easily and cleanly with the current approach.

src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Great fix @benjamn!

src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
src/core/ObservableQuery.ts Show resolved Hide resolved
Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

My small and smooth brain is unable to understand how methods like getLastResult(), resetLastResult() and isDifferentFromLastResult() are supposed to be used. How would we communicate their purpose to users who directly call client.watchQuery()?

Nevertheless, if this fixes issues for users, I guess it should be merged.

@benjamn benjamn force-pushed the avoid-voiding-result.data-in-getCurrentResult branch from f14bc92 to be29be7 Compare August 13, 2021 22:55
@brainkim
Copy link
Contributor

LGTM so I can rebase against this in #8596

This change fixes issue #8620, by not clobbering `result.data` whenever
`diff.complete` is false, but introduces other problems (test failures)
that I will solve in later commits.
This refactoring should have no observable consequences, since these are
all `private` properties of `ObservableQuery`.
I might prefer the default behavior to be reversed (variables must match
by default), but these are `public` methods of `ObservableQuery`, so we
should be careful to preserve the same behavior when `getLastResult` or
`getLastError` are called with no arguments.
This fixes #7978 in a better way than #8422, by actually checking
whether the `last.variables` used for `last.result` are still equal to
the current `options.variables`, before reusing `last.result` in
`ObservableQuery#getCurrentResult`.
This test was previously delivering a `loading: false` result with
neither `data` nor `error` defined, and now it delivers the correct
data. Progress!
This resolves the concern I raised in this comment:
#8642 (comment)
@benjamn benjamn force-pushed the avoid-voiding-result.data-in-getCurrentResult branch from be29be7 to c834089 Compare August 16, 2021 20:28
@benjamn benjamn merged commit 47c5755 into main Aug 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Previous data returned when changing variables and fetchPolicy is network-only (Reproduction Provided)
3 participants