-
Notifications
You must be signed in to change notification settings - Fork 646
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
ApolloClient.rxQuery configure lambda confusing and error prone #2430
Comments
It looks like internally it uses a toBuilder() and then build() |
I feel like the consistency of the 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? |
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)
}
} |
Tentative improvement there: #2434 It won't directly fix the |
#2434 has been merged, I'm not sure how to follow up and make the @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 |
Not sure 🤔 I don't reall have a good idea right now |
Let's wait a bit until we gather more feedback, if |
Closing this one with #2434. Will reopen if there are more reports and we need to add |
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
The text was updated successfully, but these errors were encountered: