Skip to content

[v2] [12/X] ApolloClient Fetch APIs #684

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

Open
wants to merge 3 commits into
base: v2-ClientContext
Choose a base branch
from

Conversation

AnthonyMDev
Copy link
Contributor

This pull request introduces the new public facing APIs for the fetch methods of ApolloClient. A few other changes were made to enable these new APIs.

Cache Policies

Depending on a combination of the CachePolicy and the OperationResponseFormat, ApolloClient fetch functions will either return async throws -> GraphQLResult<Query.Data> or throws -> AsyncThrowingStream<GraphQLResult<Query.Data>, any Error> . In order to enable this, the CachePolicy`` now consists of multiple enums that can be used with different ApolloClient` functions.

All of the functions have the same calling signature (fetch(query: cachePolicy: requestConfiguration:)), with only differing return types. This allows the user to call the function passing any value for any of the nested cache policy enums and Swift will automatically infer the correct return type at the call site.

NetworkTransport & RequestChain always return a stream of results, the ApolloClient functions know if they expect that stream to return multiple results or only a single result. For the single result cases, the client awaits the stream and just returns the first emitted value.

This PR also introduces FetchBehavior. This is a new struct that encapsulates the behaviors described by any of the cache policies. For ease of use, the NetworkTransport, RequestChain, and GraphQLRequest operate on the FetchBehavior. The client converts the passed cache policy into a FetchBehavior and passes it to the NetworkTransport.

Additionally a new .networkElseCache cache policy was introduced. While other cache policies have had their names changed and been restructured, no equivalent behavior previously existed.

Request Configuration

This PR also introduces the RequestConfiguration struct. This encapsulates request-specific settings such as requestTimeout and writeResultsToCache. The RequestConfiguration is passed along to the NetworkTransport, which is responsible for respecting the configuration values (typically by passing them to a constructed GraphQLRequest instance).

ApolloClient is initialized with a defaultRequestConfiguration, allowing the requestConfiguration parameter on all fetch functions to be optional with a default nil value. When no configuration is passed, the defaultRequestConfiguration is passed to the NetworkTransport.

RequestChain Behavior

The RequestChain previously would attempt a cache read (if required by the CachePolicy) before any GraphQLInterceptors were called. Meaning for a .cacheOnly request, the interceptors were never called at all. This PR changes that so that GraphQLInterceptors are called first, allowing them to mutate the request before the cache read. The cache response is now also passed back up through the interceptors prior to be emitted out of the result stream.

RequestChain was also modified to support the new .networkElseCache cache policy.

New Utility Extensions

This PR adds utility methods to AsyncThrowingStream for executing tasks and passing through results, improving asynchronous programming support.

For retrying a failed request, allowing interceptor to inspect & change the fetch behavior is useful.
@@ -76,25 +52,31 @@ public final class ApolloClient: ApolloClientProtocol, Sendable {
/// - networkTransport: A network transport used to send operations to a server.
/// - store: A store used as a local cache. Note that if the `NetworkTransport` or any of its dependencies takes
/// a store, you should make sure the same store is passed here so that it can be cleared properly.
/// - defaultCachePolicy: TODO
/// - clientAwarenessMetadata: Metadata used by the
Copy link
Member

Choose a reason for hiding this comment

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

Needing documentation for the new defaultRequestConfiguration parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've been changing some of the docs as I go, but we'll need to do a full documentation audit prior to public release.

/// - Parameter url: The URL of a GraphQL server to connect to.
/// - Parameters:
/// - url: The URL of a GraphQL server to connect to.
/// - clientAwarenessMetadata: Metadata used by the
Copy link
Member

Choose a reason for hiding this comment

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

Needing documentation for the new defaultRequestConfiguration parameter.

Same here..

@@ -26,6 +26,7 @@ public struct AutoPersistedQueryConfiguration: Sendable, Hashable {

public protocol AutoPersistedQueryCompatibleRequest: GraphQLRequest {

#warning("Consider moving this to ClientContext or RequestConfiguration?")
Copy link
Member

Choose a reason for hiding this comment

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

RequestConfiguration seems like a good home for this based on the scope/behaviour we discussed yesterday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way RequestConfiguration works, the values are just passed to the NetworkTransport which then uses them when constructing the GraphQLRequest. So if we added them there, they would still need to be here also. And I actually think it's vary unlikely right now that you would want a different apq config per request.

Probably, you want the same apq config for all requests on the same NetworkTransport. So I actually think the current setup is correct. RequestChainNetworkTransport is initialized with an apqConfig, and it passes that to each request if constructs. But having it here allows the interceptors to modify the APQ behavior of requests. I think that's what we want.


// MARK: -

/// Describes the cache/networking behaviors that should be usedfor the execution of a GraphQL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Describes the cache/networking behaviors that should be usedfor the execution of a GraphQL
/// Describes the cache/networking behaviors that should be used for the execution of a GraphQL

@@ -30,8 +32,8 @@ public protocol NetworkTransport: AnyObject, Sendable {
public protocol SubscriptionNetworkTransport: NetworkTransport {

func send<Subscription: GraphQLSubscription>(
subscription: Subscription,
cachePolicy: CachePolicy
subscription: Subscription,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subscription: Subscription,
subscription: Subscription,

public let request: Request

public init(request: Request) {
self.request = request
self.request = request
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.request = request
self.request = request

} catch {
#warning(
"""
TODO: If we are making cache miss return nil (instead of throwing error), then should
Copy link
Member

Choose a reason for hiding this comment

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

Does this reflect the thinking at the end of our conversation yesterday, or an evolution of that?

@@ -19,7 +19,7 @@ public final class RequestChainNetworkTransport: NetworkTransport, Sendable {
///
/// Defaults to an empty dictionary.
public let additionalHeaders: [String: String]

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why there is whitespace being added in places like this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants