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

Refactor ObservableQuery#getCurrentResult to reenable immediate delivery of warm cache results. #6710

Merged
merged 16 commits into from
Jul 27, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jul 27, 2020

The crucial commit is the final one, which fixes #6659 and #6686 by making the getCurrentResult method of ObservableQuery return a result with loading: false when the cache provides complete data, assuming the FetchPolicy is cache-first or cache-only (the two policies that allow for non-loading results from the cache).

The general principle here is that getCurrentResult should return either the result most recently delivered to the ObservableQuery, or (if no result has been delivered yet) the next result that will be delivered. In cases where the cache is already primed with the necessary data (perhaps by a parent UI component), that next result can have loading: false, so it's important that getCurrentResult also returns a loading: false result in those cases.

Since queryInfo.networkStatus defaults to NetworkStatus.loading when QueryInfo objects are first initialized in QueryManager#watchQuery, getCurrentResult code was confused by queryInfo.networkStatus, and missed the opportunity to deliver non-loading results when the cache is warm.

I've never been happy with getCurrentResult and getCurrentQueryResult, so I also took this opportunity to unify those methods and simplify the surrounding logic.

benjamn added 14 commits July 27, 2020 10:36
Reducing the number of calls to getCurrentQueryResult from two to one will
make it easier to collapse getCurrentQueryResult into getCurrentResult.
Managing cache watching is now the exclusive responsibility of the
QueryInfo class.
Since no-cache queries never read from the cache, there's no point in
making them watch the cache for changes, because the cache doesn't know
anything about which fields were used by the no-cache query.
This gives the ObservableQuery direct access to its corresponding
QueryInfo object, so the ObservableQuery doesn't have to keep asking the
QueryManager for the QueryInfo corresponding to this.queryId.

Important invariants: given ObservableQuery oq and QueryManager qm, it
should always be the case that qm.queries.get(oq.queryId) ===
oq.queryInfo, and oq.queryInfo.observableQuery === oq.
Since ObservableQuery objects have a direct reference to their QueryInfo
objects, it's possible for an ObservableQuery to find use for these fields
even after the QueryManager has stopped the query and removed the
QueryInfo object from queryManager.queries.
PR #6353 explains the rationale for switching to a cache-first FetchPolicy
after an initial cache-and-network or network-only policy.

When #6365 was implemented, options.fetchPolicy was examined only once, at
the beginning of fetchQueryObservable, so the timing of changing
options.fetchPolicy did not matter as much. However, fixing #6659 involves
checking the current options.fetchPolicy whenever the QueryData class
calls this.currentObservable.getCurrentResult(), so it's now more
important to delay changing options.fetchPolicy until after the first
network request has completed.
Comment on lines -205 to +174
if (partial) {
this.resetLastResults();
} else {
this.updateLastResult(result);
}
this.updateLastResult(result);
Copy link
Member Author

Choose a reason for hiding this comment

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

Always updating this.lastResult makes more sense now that ApolloQueryResult contains the partial boolean field. We last considered this logic in #6417.

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.

Fantastic @benjamn - everything looks great, and thanks very much for retiring ApolloCurrentQueryResult at the same time! 🙇‍♂️

@jimrandomh
Copy link

jimrandomh commented Jul 27, 2020

Thanks for this! I'll give this branch a try (and I expect several others will as well).

There's one more case, which a quick skim of the code suggests might not be handled correctly: when fetchPolicy is cache-and-network, but it isn't going to be fetched because of ssrForceFetchDelay or disableNetworkFetches (as described in #4814).

@trompx
Copy link

trompx commented Sep 11, 2020

In which version is this supposed to be fixed? With version 3.1.5 and this:

const { user, loading } = useMe();
if (loading) {
  return <>Loading</>;
}

When I reload the pageI get the warning Warning: Text content did not match. Server: "Hello" Client: "Loading"
If I switch to:

const { user, loading } = useMe();
if (!user) {
  return <>Loading</>;
}

like mentionned here, that works

I also don't get the warning if I set apollo client fetchPolicy to no-cache.
Update : just tried version v3.2.0-rc.0 and still having this issue.

@arvindell
Copy link

@trompx I am experiencing the same behavior in AC3.3

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
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.

v3.0: loading: true for first render always, even if all data is available in cache
5 participants