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

Do not cancel server fetch of watcher on dependent key update #1156

22 changes: 19 additions & 3 deletions Sources/Apollo/GraphQLQueryWatcher.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,14 @@ public final class GraphQLQueryWatcher<Query: GraphQLQuery>: Cancellable, Apollo
fetch(cachePolicy: .fetchIgnoringCacheData)
}

// Watchers always call result handlers on the main queue.
private let callbackQueue: DispatchQueue = .main

func fetch(cachePolicy: CachePolicy) {
// Cancel anything already in flight before starting a new fetch
fetching?.cancel()
fetching = client?.fetch(query: query, cachePolicy: cachePolicy, context: &context, queue: .main) { [weak self] result in
guard let `self` = self else { return }
fetching = client?.fetch(query: query, cachePolicy: cachePolicy, context: &context, queue: callbackQueue) { [weak self] result in
guard let self = self else { return }

switch result {
case .success(let graphQLResult):
Expand All @@ -66,7 +69,20 @@ public final class GraphQLQueryWatcher<Query: GraphQLQuery>: Cancellable, Apollo
guard let dependentKeys = dependentKeys else { return }

if !dependentKeys.isDisjoint(with: changedKeys) {
fetch(cachePolicy: .returnCacheDataElseFetch)
// First, attempt to reload the query from the cache directly, in order not to interrupt any in-flight server-side fetch.
store.load(query: query) { [weak self] result in
guard let self = self else { return }

switch result {
case .success:
self.callbackQueue.async { [weak self] in
self?.resultHandler(result)
}
case .failure:
// If the cache fetch is not successful, for instance if the data is missing, refresh from the server.
self.fetch(cachePolicy: .fetchIgnoringCacheData)
Copy link
Contributor

Choose a reason for hiding this comment

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

is .fetchIgnoringCacheData correct here? it was .returnCacheDataElseFetch before.
Can we get into some fetch cycle here? fetch(cachePolicy: CachePolicy) -> Fail -> fetch(cachePolicy: CachePolicy) -> Fail -> ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Before it was returnCacheDataElseFetch because we weren't explicitly trying to load from the cache. By the point this gets hit, we've already checked the cache so fetchIgnoringCacheData acknowledges that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the fail is for reading directly from the store, rather than for hitting the network, so that cycle should not be an issue.

}
}
}
}
}