-
Notifications
You must be signed in to change notification settings - Fork 64
[v2] [8/X] Add OperationResponseFormat #677
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
base: v2
Are you sure you want to change the base?
Conversation
Another PR will implement ensuring that generated operations with @defer use the IncrementalDeferredResponseFormat
#warning("TODO: rethink this API. Only valid for queries currently") | ||
public enum CachePolicy: Sendable, Hashable { | ||
/// Return data from the cache if available, else fetch results from the server. | ||
case returnCacheDataElseFetch | ||
/// Always fetch results from the server. | ||
case fetchIgnoringCacheData | ||
/// Always fetch results from the server, and don't store these in the cache. | ||
case fetchIgnoringCacheCompletely | ||
/// Return data from the cache if available, else return an error. | ||
case returnCacheDataDontFetch | ||
case cacheFirst | ||
/// Attempt to fetch results from the server, if failed, return data from the cache if available. | ||
case networkFirst | ||
/// Fetch results from the server, do not attempt to read data from the cache. | ||
case networkOnly | ||
|
||
/// Return data from the cache if available, and always fetch results from the server. | ||
case returnCacheDataAndFetch | ||
case cacheAndNetwork // seperate function? | ||
|
||
/// Return data from the cache if available, else return an error. | ||
case cacheOnly // replace with separate function? | ||
|
||
#warning("TODO: this unsafe is not properly made atomic. Fix this") | ||
/// The current default cache policy. | ||
nonisolated(unsafe) public static var `default`: CachePolicy = .returnCacheDataElseFetch | ||
/// Always fetch results from the server, and don't store these in the cache. | ||
// case fetchIgnoringCacheCompletely | ||
|
||
#warning("TODO: this unsafe is not properly made atomic. Fix this") | ||
// /// The current default cache policy. | ||
// nonisolated(unsafe) public static var `default`: CachePolicy = .cacheFirst |
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.
All changes in ApolloClient
itself are just for POC and are still in flux.
The plan is to only include those first three cache policies.
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.
New CachePolicy API is implemented in #684
struct RequestConfigurationContext { | ||
@TaskLocal static var taskLocal: RequestConfigurationContext = .init() | ||
|
||
var requestTimeout: Int32 = 30 | ||
var writeResultsToCache: Bool = 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 something I'm toying with, but I'm not sure if it will make the final release. This probably shouldn't even be here right now, but a lot of stuff is under construction at the same time, and I did want to get the primary components of this PR up for discussion. Happy to discuss this here, but it will be finalized (or removed) in a later PR, so let's not let this hold up approving this one.
This adds a new RequestConfigurationContext
that provides a TaskLocal
value. This can replace the usage of RequestContext
for timeout configuration (RequestContextTimeoutConfigurable
) and other future configuration options.
This configuration context also currently includes a writeResultsToCache
property. The intention is that CachePolicy
can be used only for indicating if we should fetch from the cache vs network (and only used for query operations). While configuration of writing results to the cache could be done using this task local value.
Because users can also use their own custom task local values anywhere they want, I'm actually considering if we can remove RequestContext
completely.
The primary issue I see with this new approach is that I haven't found a great way to allow users to decide what the initial default values are for the configuration properties. If you want to change one of these values for a single operation, it's straightforward:
var requestConfig = Apollo.RequestConfigurationContext.taskLocal // would probably change the name of this property
requestConfig.writeResultsToCache = false
Apollo.RequestConfigurationContext.$taskLocal.withValue(requestConfig) {
let result = client.fetch(query: myQuery)
...
}
But if you wanted to, for instance, change the default requestTimeout
for all of your operations, I haven't come up with a great way to do that yet.
I'm thinking that maybe you can initialize ApolloClient
with a default request configuration and then use the TaskLocal version to override the default when you want to.
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.
We discussed this earlier today. Should be some changes incoming, like #680, from pairing on that.
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.
Yes, this has been changed and the new APIs for this, which are included in #684 do not use @TaskLocal
values.
The configuration option currently using @TaskLocal
is ClientAwarenessMetadata
which is implemented in #681. After discussion with Calvin, we feel that using @TaskLocal
only on internal
values is acceptable.
public func fetch<Query: GraphQLQuery>( | ||
query: Query, | ||
cachePolicy: CachePolicy = .default, | ||
context: (any RequestContext)? = nil, | ||
queue: DispatchQueue = .main, | ||
resultHandler: GraphQLResultHandler<Query.Data>? = nil | ||
) -> (any Cancellable) { | ||
return awaitStreamInTask( | ||
{ | ||
try self.networkTransport.send( | ||
query: query, | ||
cachePolicy: cachePolicy, | ||
context: context | ||
) | ||
}, | ||
callbackQueue: queue, | ||
completion: resultHandler | ||
) | ||
cachePolicy: CachePolicy? = nil, | ||
context: (any RequestContext)? = nil | ||
) async throws -> GraphQLResult<Query.Data> | ||
where Query.ResponseFormat == SingleResponseFormat { | ||
for try await result in try self.networkTransport.send( | ||
query: query, | ||
cachePolicy: cachePolicy ?? self.defaultCachePolicy, | ||
context: context | ||
) { | ||
return result | ||
} | ||
throw ApolloClientError.noResults | ||
} | ||
|
||
@available(*, deprecated) | ||
private func awaitStreamInTask<T: Sendable>( | ||
_ body: @escaping @Sendable () async throws -> AsyncThrowingStream<T, any Swift.Error>, | ||
callbackQueue: DispatchQueue?, | ||
completion: (@Sendable (Result<T, any Swift.Error>) -> Void)? | ||
) -> some Cancellable { | ||
let task = Task { | ||
do { | ||
let resultStream = try await body() | ||
|
||
for try await result in resultStream { | ||
DispatchQueue.returnResultAsyncIfNeeded( | ||
on: callbackQueue, | ||
action: completion, | ||
result: .success(result) | ||
) | ||
} | ||
|
||
} catch { | ||
DispatchQueue.returnResultAsyncIfNeeded( | ||
on: callbackQueue, | ||
action: completion, | ||
result: .failure(error) | ||
) | ||
} | ||
} | ||
return TaskCancellable(task: task) | ||
public func fetch<Query: GraphQLQuery>( | ||
query: Query, | ||
cachePolicy: CachePolicy? = nil, | ||
context: (any RequestContext)? = nil | ||
) throws -> AsyncThrowingStream<GraphQLResult<Query.Data>, any Error> | ||
where Query.ResponseFormat == IncrementalDeferredResponseFormat { | ||
return try self.networkTransport.send( | ||
query: query, | ||
cachePolicy: cachePolicy ?? self.defaultCachePolicy, | ||
context: context | ||
) |
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 a POC of how we can use the new Query.ResponseFormat
to determine what the return value for a fetched query will be. Not a finalized API yet.
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.
Finalized fetch APIs in #684
@@ -27,7 +27,7 @@ public protocol ApolloClientProtocol: AnyObject { | |||
/// - resultHandler: [optional] A closure that is called when query results are available or when an error occurs. |
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.
ApolloClientProtocol
will be updated once I've finalized the API for ApolloClient
(or maybe moved into ApolloTestHelpers
?)
case fetchIgnoringCacheCompletely | ||
/// Return data from the cache if available, else return an error. | ||
case returnCacheDataDontFetch | ||
case cacheFirst |
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 wanted to comment that cacheFirst
sounds like a different default behaviour than returnCacheDataElseFetch
but on second thought I don't think the intent of the cache policy has actually changed, the documentation for the case is still the same. That leads me to think that we need to consider another name.
returnCacheDataElseFetch
is mutually exclusive, i.e.: one or the other will happen, not both.cacheFirst
sounds like there is a secondary action, i.e.: cache first, then network.
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.
If the case name changes from this then we should take a second look at the rest too.
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.
That's fair. For the work I'm doing right now, I have predefined FetchBehavior
values called CacheThenNetwork
and CacheElseNetwork
. I'm open to changing this to `.cacheElseNetwork.
The reason why I went with cacheFirst
and networkFirst
was to align with the cache policy definitions in ApolloKotlin.
@martinbonnin Do you think that cacheFirst
has been confusing to users in any way?
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 decided to go with something more specific and clear for all of the cache policy values. This is now implemented in #684.
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.
@AnthonyMDev we haven't had any issue with CacheFirst
. This is also what Apollo Client is using and I think quite established today. If you can, I'd love to get those aligned between different platforms.
Ping @BoD
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.
Yeah, I don't think it's too confusing, but I do agree with Calvin that .cacheElseNetwork
,.cacheThenNetwork
, and .networkElseCache
are clearer.
I prefer the clearer versions, but if we want to weigh parity of the naming across platforms higher than iOS team's preference on naming, I'm okay with that. I just want to make a decision today and move forward.
My feeling is that, while naming parity is nice, it's not something that we have consistently in other areas of the libraries currently. If we were aligned on naming in most of the library, then I'd try to stick with that, but not sure if there is much value in doing that here if we aren't consistent with it.
@bignimbus Do you have any opinion on this?
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 do like the more clear names, although I think having more parity between the platforms is a good thing. If this specific case is a big issue to have or not have parity could go either way. But I think not having parity just because we don't have it in other areas is a reason that could basically always be used and would end up being a roadblock in working towards for parity going forward.
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 fine with going the parity route. I do think more alignment across the clients is a good direction to move towards.
public init(deferredFragments: [DeferredFragmentIdentifier: any SelectionSet.Type]) { | ||
self.deferredFragments = deferredFragments | ||
} | ||
} |
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 think we're missing a response format for HTTP subscriptions? They're closely related to IncrementalDeferredResponseFormat
but not defined by the use of @defer
.
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.
You're right! I'll be getting to that soon in another PR.
In 2.0, we want
ApolloClient
to have functions with different return values based on if we know there will be a single returned result vs when we expect multiple responses. We can expect multiple responses when the operation includes a@defer
directive OR when fetching from both the cache and the network (ie. 1.0CachePolicy.returnCacheDataAndFetch
).In order to support these different functions this PR adds
OperationResponseFormat
toGraphQLOperation
. Now that operations are generated with thisassociatedtype
, they can indicate if they use aSingleResponseFormat
or anIncrementalDeferredResponseFormat
. Other response formats can be added in the future (for@stream
possibly).This allows us to create a variety of functions with different single/multiple return types on
ApolloClient
with constrainingwhere
clauses.