Uniformize operation APIs?

[martinbonnin]

Currently we have:

apolloClient.query(query).execute()
apolloClient.mutate(mutation).execute()
apolloClient.subscribe(subscription).execute()

I find it weird, especially in the mutation/subscription case we have 2 chained verbs. Should we do this instead?

apolloClient.query(query).execute()
apolloClient.mutation(mutation).execute()
apolloClient.subscription(subscription).execute()

or

apolloClient.queryCall(query).execute()
apolloClient.mutationCall(mutation).execute()
apolloClient.subscriptionCall(subscription).execute()

or

apolloClient.query(query).await()
apolloClient.mutate(mutation).await()
apolloClient.subscribe(subscription).toFlow()

Given that 3.0 is coming soon, it's now or never

[BoD]

I like this change (favoring the 1st one without "call") because it mirrors the GraphQL syntax.

[martinbonnin]

Silly question. Why not keeping the 2.x API? (added also in the main description as option 3.)

apolloClient.query(query).await()
apolloClient.mutate(mutation).await()
apolloClient.subscribe(subscription).toFlow()

[BoD]

Do you mean as a (deprecated) migration helper?

[martinbonnin]

Do you mean as a (deprecated) migration helper?

Not really. What about keeping it for real? Except it doesn't need the imports?

[BoD]

So they would be a synonym of execute? Or do you mean rename execute to use these names instead?

[martinbonnin]

Or do you mean rename execute to use these names instead?

That.

apolloClient.mutate(mutation).execute() feels weird but apolloClient.mutate(mutation).await() sounds better to me. The first verb (mutate) starts the action, the second (await) awaits completion. As a bonus, it reuses the same API as 2.x

[BoD]

I see!

I personally prefer execute because of its consistency across the 3 operations (less surprising API) but it's pretty much subjective, and I see your point too. One thing: not sure it would be clear that await can be called multiple times whereas it makes sense (at least to me!) with execute.

[martinbonnin]

consistency across the 3 operations (less surprising API)

The counter argument can be made that subscriptions are of different kind (Flow) so could use different name. But yea...

not sure it would be clear that await can be called multiple times whereas it makes sense (at least to me!) with execute

100% agree, that's a good point.

Also now is certainly a good time to think of what this API will look like under defer/stream. What we decide now would have to be somewhat compatible.

[martinbonnin]

One more argument for mutation instead of mutate is that it makes it less tempting to omit the return value if it's not read:

// Might be tempting to call it a day and assume something changed (doesn't work)
apolloClient.mutate(mutation)

// `mutation` is not an action so it's a hint that execute() is required
apolloClient.mutation(mutation).execute()

PS: can we add an annotation to warn if the return value isn't used?

[BoD]

Just a reminder of the current situation:

ApolloCall<D: Operation.Data> (abstract)
fun executeAsFlow(): Flow<ApolloResponse<D>>

ApolloQueryCall<D: Query.Data>
suspend fun execute(): ApolloResponse<D>

ApolloMutationCall<D: Mutation.Data>
suspend fun execute(): ApolloResponse<D>

ApolloSubscriptionCall<D: Subscription.Data>
fun execute(): Flow<ApolloResponse<D>>

It's still very early on the reflection about defer/stream but it seems to me that we could already go for this naming convention:

• executeAsFlow returns Flow
• execute returns 1 result (suspend)

This means in the future, for queries and mutations, partial results could be obtained by calling executeAsFlow while "traditional" (not defer/stream) can use execute

This also means execute on ApolloSubscriptionCall should probably be deleted for consistency (consumers should use executeAsFlow instead).

WDYT?

[martinbonnin]

This makes sense. I'm all for deleting ApolloSubscriptionCall.execute() (after a short deprecation period...).

About executeAsFlow, I'm a bit concerned that execute() and Flow are somewhat contradictory. execute is terminal while flow are cold and need to be collected. Maybe

• toFlow() returns Flow
• execute() suspends and returns 1

This is relatively similar to what SQLDelight is doing with Query.asFlow (except they're using as and not to)

[BoD]

Good point about the execute! For to vs as, no real preference for now even though I tend to favor to for no particular reason 😅

So it would look like this in beta-04:

ApolloCall<D: Operation.Data> (abstract)
@Deprecated("use toFlow")
fun executeAsFlow(): Flow<ApolloResponse<D>>
fun toFlow(): Flow<ApolloResponse<D>>

ApolloQueryCall<D: Query.Data>
suspend fun execute(): ApolloResponse<D>

ApolloMutationCall<D: Mutation.Data>
suspend fun execute(): ApolloResponse<D>

ApolloSubscriptionCall<D: Subscription.Data>
@Deprecated("use toFlow")
fun execute(): Flow<ApolloResponse<D>>

In a future version, we may need to add toPartialFlow (for instance) to handle stream/defer...

[BoD]

About checking the return values, I dug a bit:

• there is a CheckReturn annotation in Android but we probably don't want to depend on this
• there is an issue asking to add a similar one in Kotlin
• it looks like (see screenshot) any annotation named *CheckReturnValue would trigger the warning in IntelliJ/AS
• Rx did this for instance so we could do this ourselves too

[martinbonnin]

• 1 on not depending on Android annotations.

Not strong opinion about having ApolloCheckReturnValue. Let's wait until we have a question/use case before adding it?

[BoD]

Closing per #3594