Should ApolloClient.readQuery have it's options defined as DataProxy.readQueryOptions?

[baseten]

The implementation of ApolloClient.readQuery defines its options as DataProxy.Query like so:

public readQuery<T = any, TVariables = OperationVariables>(
    options: DataProxy.Query<TVariables, T>,
    optimistic: boolean = false
  ): T | null {
    return this.cache.readQuery<T, TVariables>(options, optimistic);
  }

https://github.com/apollographql/apollo-client/blob/main/src/core/ApolloClient.ts#L415-L420

However the underlying DataProxy defines ReadQueryOptions like so:

 export interface ReadQueryOptions<TData, TVariables>
    extends Query<TVariables, TData> {
    /**
     * Whether to return incomplete data rather than null.
     * Defaults to false.
     */
    returnPartialData?: boolean;
    /**
     * Whether to read from optimistic or non-optimistic cache data. If
     * this named option is provided, the optimistic parameter of the
     * readQuery method can be omitted. Defaults to false.
     */
    optimistic?: boolean;
    /**
     * Whether to canonize cache results before returning them. Canonization
     * takes some extra time, but it speeds up future deep equality comparisons.
     * Defaults to false.
     */
    canonizeResults?: boolean;
  }

https://github.com/apollographql/apollo-client/blob/main/src/cache/core/types/DataProxy.ts#L58-L77

Is this done intentionally to prevent ApolloClient.readQuery from receiving the full cache options? Currently (although not type-safe), it's possible to pass returnPartialData: true to client.readQuery. This would be super useful for our use case, but we don't want to use it if it's an unintentional escape hatch.

[jerelmiller]

Hey @baseten 👋

This appears to be more of an oversight rather than intentional restriction. Since client.readQuery just forwards the call to client.cache.readQuery, there is no reason those types shouldn't line up. Updating to DataProxy.ReadQueryOptions is a backwards compatible change, so this should be safe to change.

Would you be willing to open a PR to update the type here? We'd be happy to accept it.

One note on a small tweak I'd make when doing so. Notice how DataProxy.ReadQueryOptions accepts an optimistic option, yet our client.readQuery function has an optimistic argument as the 2nd argument. For backwards compatibility, we'll want to leave this alone, but to avoid confusion I think we should omit optimistic from options in the type since the optimistic argument is going to override anything in options.

In other words, the type should be:

public readQuery<T = any, TVariables = OperationVariables>(
  options: Omit<DataProxy.ReadQueryOptions<TVariables, T>, 'optimistic'>,
  optimistic: boolean = false
): T | null {
  return this.cache.readQuery<T, TVariables>(options, optimistic);
}

Let me know if you'd be willing to open a PR!