apollographql / apollo-kotlin

:rocket:  A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.76k stars 653 forks source link

Make ApolloStore aware of the client CustomScalarAdapters #3823

Open glung opened 2 years ago

glung commented 2 years ago

Summary

When using custom scalars, apolloStore#readOperations and apolloStore#writeOperations may throw an IllegalStateException.

This is more an API change request than a real bug (see below)

Version 3.0.0

Description

This is because the custom scalar adapters known to the client, are not known by the store. The client passes down their custom scalar adapters when writing to the store. When accessing directly the store, the custom scalar adapters are not known (it's an optional parameter of readOperation & writeOperation).

Below are code snippets:

appoloClient.apolloStore.readOperation(operation = query)

-> Can't map GraphQL type: `${customScalar.name}` to: `${customScalar.className}`. Did you forget to add a CustomScalarAdapter?

instead

    appoloClient.apolloStore.readOperation(
        operation = query,
        customScalarAdapters = appoloClient.customScalarAdapters
    )
BoD commented 2 years ago

Hi! I think it was probably on purpose that the store and client are kind of independent / well isolated as a general good practice, but it's true that the use of custom scalars may not be the most intuitive.

An extension on the client could be something like this,

fun ApolloClient.readFromStore(operation: Operation) = apolloStore.readOperation(operation, customScalarAdapters)

but that would be the equivalent of: executing the query with a CacheOnly fetchPolicy, so I'm not sure it would be very useful?

glung commented 2 years ago

but that would be the equivalent of: executing the query with a CacheOnly fetchPolicy, so I'm not sure it would be very useful?

Good point but there is no equivalent in the client's API for writeOperation.

About the API, it's was indeed unexpected for me that the store attached to the client would not know about the custom scalars adapters for the data it's effectively storing.

Other alternative options:

martinbonnin commented 2 years ago

It makes sense to store the scalar adapters in ApolloStore. I think the main issue is initialization order since typically the ApolloStore is created before any call to addCustomScalarAdapter is made. Ideally, we would move the ApolloStore construction to ApolloClient.Builder.build() and pass it the adapters at that time. But since apollo-runtime doesn't know anything about the cache, we'd need to find a generic way to do this.

In all cases, updating the doc sounds like a good thing to do 👍

martinbonnin commented 2 years ago

Doc has been updated.

I'm keeping this issue open with a new title to keep track of the API change request. Most likely for the next major version

martinbonnin commented 2 years ago

We have the same issue with the Dispatcher. See this kotlinlang thread