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
GraphQLQueryWatcher: publisher
property
#346
Conversation
👷 Deploy request for eclectic-pie-88a2ba pending review.Visit the deploys page to approve it
|
👷 Deploy request for apollo-ios-docc pending review.Visit the deploys page to approve it
|
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'm not opposed to this addition, but I want to make sure we think through it.
I'm unclear on how easy it is to create a retain cycle here. There will definitely be a retain on the watcher, but since the watcher holds on to the subscriptions
will that cause a retain cycle in any situation? What if the subscriber block accesses the watcher
itself? I guess that would be broken by calling cancel
though, right?
query: Query, | ||
context: RequestContext? = nil, | ||
callbackQueue: DispatchQueue = .main, | ||
handler: GraphQLResultHandler<Query.Data>? |
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 would think the handler would need to be @escaping
here also. What am I missing?
public typealias Output = Result<GraphQLResult<Query.Data>, Error> | ||
public typealias Failure = Never | ||
|
||
var watcher: GraphQLQueryWatcher |
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.
Thinking this should be a let
right?
|
||
let watcher = GraphQLQueryWatcher(client: client, query: watchedQuery, resultHandler: resultObserver.handler) | ||
var results: [GraphQLQueryWatcher<MockQuery<SimpleMockSelectionSet>>.CachePublisher.Output] = [] | ||
var subscription = watcher.publisher().sink { result in |
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.
Make this let
please.
🤔 Imagine the following scenario: A user takes five rapid actions, each triggering an optimistic cache update and queuing a network request which makes the mutation. The mutation responds with the updated data, and in the event of failure rolls back the optimistic update. The user actions must have optimistic updates attached, as the UI would otherwise be out-of-sync. The server responses, however, would contain stale data relative to other optimistic updates – which would blow out the user's changes, resulting in a jarring experience. (Especially if the user was deleting/removing items from a list – like when we archive emails) This PR attempts to solve this problem by giving consumers of a watcher the ability to debounce or throttle outputs. While that may be a desired behavior in another context, it seems like it shouldn't be the primary solution. Instead, what if each updated value had an func perform<Mutation: GraphQLMutation, T>(
mutation: Mutation,
publishResultToStore: Bool,
context: RequestContext?,
queue: DispatchQueue,
optimisticUpdate: ((ReadWriteTransaction) throws -> T)?, // NEW: optional `optimisticUpdate` parameter
resultHandler: GraphQLResultHandler<Mutation.Data>?
) -> Cancellable This way, the client can:
With all of this in mind, the (success) path would look something like this: A user takes five rapid actions, each triggering an optimistic cache update and queuing a network request which makes the mutation. Each optimistic event is tied to a given timestamp, and only data whose timestamp is equal (or later than) the previous data's timestamp can overwrite. Thus, when a mutation responds, it only overwrites data that has no timestamp (existing cached data) or confirms the optimistic update by overwriting data with a timestamp equal-to-or-less-than its timestamp. After five mutations, the cache state remains correct throughout the process, even for actions that have not yet been confirmed by the server. If the consumer of the API wants throttling or debouncing, that seems like a separate (and separately solved) problem. There's still the matter of rollbacks, and that's something to think about in this API. We should have a way of exposing a rollback to the user: this is a common use-case and I'm certain we can take notes from how What do you think, @AnthonyMDev? |
What are you trying to accomplish?
I noticed that in situations where we have a large number of local cache updates and network cache updates, that we end up refreshing the UI far too often. By being able to treat the results of cache updates as a
Publisher
, we can placethrottle
ordebounce
functionality into the watcher's update stream.What approach did you choose and why?
This PR introduces the following changes:
apollo-ios/Sources/Apollo/GraphQLQueryWatcher.swift
: TheGraphQLQueryWatcher
class was extended to include aCachePublisher
struct and aWatcherSubscription
class, enabling the watcher to publish cache updates to subscribers. Apublisher()
method was also added to allow access to theCachePublisher
.GraphQLQueryWatcher
aPublisher
itself, as it is an observer of the underlying cache.apollo-ios/Sources/Apollo/GraphQLQueryWatcher.swift
: TheGraphQLQueryWatcher
class was refactored to include convenience initializers and a private initializer. TheresultHandler
was updated to trigger subscriptions when a result is received.publisher
property.apollo-ios/Sources/Apollo/GraphQLQueryWatcher.swift
: TheresultHandler
property in theGraphQLQueryWatcher
class was changed from a constant to an optional variable.apollo-ios/Package.swift
: The minimum macOS version for the Apollo package was updated from 10.14 to 10.15. This is becauseCombine
is unsupported on 10.14. This constitutes a breaking change for macOS 10.14 deployments.Anything you want to highlight for special attention from reviewers?
Employing this on the
GraphQLQueryWatcher
feels like the correct choice, as opposed to doing it in a roundabout manner on theApolloClient
, but would require a version minimum increase to macOS 10_15.NOTE: I didn't tackle any macOS 10.15 deprecations.
Alternatives Considered:
ApolloCombine
project: an extension onApolloClient
. I chose to avoid this route because: it wouldn't integrate well withApolloPagination
, it would be wasteful if you want multiple subscribers on the same watcher.GraphQLQueryWatcher
into a protocol / creating a subclass ofGraphQLQueryWatcher
that has this behavior. I chose not to do this since it seems less discoverable, and perhaps a bit arbitrary. It has the advantage of not needing a version bump.