Apollo-android: ApolloClient.rxQuery configure lambda confusing and error prone

[hameno]

Summary
It is not clear from documentation and API usage, that you need to chain you calls inside the configure lambda. Normally with this kind of API you only configure the builder and it returns the same instance of the builder.
This lead to at least one bug in our code, becuase we were not using the results of the methods (e.g. responseFetcher)

Version
2.2.2

Description
see above

[hameno]

It looks like internally it uses a toBuilder() and then build()
I think it would be better to provide the builder instance to the lambda

[martinbonnin]

I feel like the consistency of the builder()/configure patterns can definitely be improved. Definitely something to keep in mind for apollo-runtime-kotlin, I'm not sure how much of it can be changed in the existing RX extensions without breaking the API. Maybe with some overloads/deprecation...

To make things explicit, can you give an exemple of code that doesn't work as expected and how you'd expect it to be written instead?

[hameno]

I had the following (wrong) code:

apolloClient.rxQuery(this) {
    responseFetcher(responseFetcher)
    if (cacheHeaders != null) {
        cacheHeaders(cacheHeaders)
    } else this
}

I think this style would have been easier to understand:

apolloClient.rxQuery(this) { builder ->
    builder.responseFetcher(responseFetcher)
    if (cacheHeaders != null) {
        builder.cacheHeaders(cacheHeaders)
    }
}

or also possible:

apolloClient.rxQuery(this) { // this: Builder
    responseFetcher(responseFetcher)
    if (cacheHeaders != null) {
        cacheHeaders(cacheHeaders)
    }
}

[martinbonnin]

Tentative improvement there: https://github.com/apollographql/apollo-android/pull/2434

It won't directly fix the rxQuery usages but will encourage to use toBuilder() to mutate values of ApolloCall

[martinbonnin]

2434 has been merged, I'm not sure how to follow up and make the rxQuery API much better. We _could_ find a new name and deprecate rxQuery:

@Deprecated("use rx3Query instead")
inline fun <D : Operation.Data, T, V : Operation.Variables> ApolloClient.rxQuery(
    query: Query<D, T, V>,
    configure: ApolloQueryCall<T>.() -> ApolloQueryCall<T> = { this }
)

inline fun <D : Operation.Data, T, V : Operation.Variables> ApolloClient.rx3Query(
    query: Query<D, T, V>,
    configure: ApolloQueryCall.Builder<T>.() -> ApolloQueryCall.Builder<T> = { this }
)

Is there a convention somewhere about when to use receiver vs parameter?

All in all, that would work but I hope we're not forgetting anything because once we've burnt rxQuery and rx3Query, there won't be a ton of other possibilities left 😅

[hameno]

Not sure 🤔 I don't reall have a good idea right now

[martinbonnin]

Let's wait a bit until we gather more feedback, if rx3Query looks good, we'll set it in API stone.

[martinbonnin]

Closing this one with #2434. Will reopen if there are more reports and we need to add rx3Query