Skip to content
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

[RFC] with-ers vs Builders vs DSL-like APIs #3301

Closed
martinbonnin opened this issue Aug 12, 2021 · 5 comments
Closed

[RFC] with-ers vs Builders vs DSL-like APIs #3301

martinbonnin opened this issue Aug 12, 2021 · 5 comments
Assignees

Comments

@martinbonnin
Copy link
Contributor

martinbonnin commented Aug 12, 2021

The main entry points to Apollo Android are ApolloClient and ApolloRequest and support multiple configuration options.

The goal of this issue is to investigate different Builder-like APIs and gather community feedback about which one feels the more natural.

1.with-ers

This is the current solution. An instance is created first and then copied with additional parameters using withFoo() methods:

val apolloRequest = ApolloRequest(query)
                                  .withFetchPolicy(FetchPolicy.CacheFirst)

val apolloClient = ApolloClient("https://example.com/graphql")
                                  .withNormalizedCache(MemoryCache())
                                  .withAutoPersistedQueries()

pros:

  • simple to implement.
  • immutable

cons:

  • some (to be measured) performance penalty due to the additional copy
  • forgetting to use the return type of a withFoo() is an error that is not caught by the compiler. It might be mitigated with an @CheckReturnValue annotation (TBC)
  • not very widespread? (TBC)

2. Builders

This is the same as Apollo Android 2.x and uses Java-style builders:

val apolloRequest = ApolloRequest.builder(query)
                                  .fetchPolicy(FetchPolicy.CacheFirst)
                                  .build()
                                  
val apolloClient = ApolloClient.builder("https://example.com/graphql")
                                  .normalizedCache(MemoryCache())
                                  .autoPersistedQueries()
                                  .build()

pros:

  • this pattern has been used for decades so most likely won't be surprising
  • doesn't copy classes
  • forgetting to use the return value is not possible by construction

cons:

  • some more boilerplate to implement as there is now a second mutable Builder class with mostly the same properties as the original one.
  • slightly more verbose, requires a final build() call

3. DSL-like

This is a solution used by Ktor, Anko and other Kotlin DSL-like APIs. The Kotlin doc has a page for them called Type-safe builders

val apolloRequest = ApolloRequest(query) {
                                    fetchPolicy = FetchPolicy.CacheFirst
                                  }
                                  
val apolloClient = ApolloClient("https://example.com/graphql") {
                                    normalizedCache {
                                      cache = MemoryCache()
                                      objectIdGenerator = IdObjectIdGenerator
                                    }
                                    autoPersistedQueries = true
                                  }

pros:

  • relatively concise
  • doesn't copy classes
  • forgetting to use the return value is not possible by construction

cons:

  • some more boilerplate to implement as there is now a second mutable Builder class with mostly the same properties as the original one.
  • slightly more verbose, requires a {} block
  • best practices about this convention do not seem as established as the Builder ones? For example, HTML uses lowercase html while Ktor uses CamelCase HttpClient
  • Not Java Friendly

4. default arguments

One option is to use the Kotlin default arguments and let users configure only those options that they need but this wasn't even in the original proposal. I'm adding this now for reference but it has a number of limitations:

  • Not Java friendly (because @JvmOverload doesn't allow all combinations of arguments)
  • Harder to evolve (because adding a parameter will be a binary breaking change)
  • Impossible to contribute parameters outside the module that defines the constructor

Bonus: ApolloRequest vs ApolloCall

Related to the above discussion and independently on the chosen solution (assumed as .configure() below for clarity), there's a question whether we should configure on an ApolloRequest or an ApolloCall.

ApolloRequest is OkHttp like:

val apolloRequest = ApolloRequest(query).configure()

val response = apolloClient.query(apolloRequest)

ApolloCall would allow to easily chain calls:

val response = apolloClient.query(query)
                         .configure()
                         .execute()

Resources/Links

Conclusion

My early thoughts would be to go with Builders because it's a battle tested solution and forces using the return value by construction. But I'd like to avoid changing that too much so will leave this ticket open for some time before comitting to Builders to make sure we're not forgetting anything.

Any thoughts? Preferences? Other solutions? Leave a comment below!

@audkar
Copy link

audkar commented Sep 7, 2021

For me, DSL looks most natural. (probably biggest cons is for library devs who need to implement it).

2nd place is for the good old Builder pattern. But I don't have a very strong preference between them. Both are good.

However with-ers I don't like at all. When using such API I don't know if that mutates the original instance or deep clones it. Is it safe to reuse or not.

@jfspencerAngel
Copy link

I have a strong preference for the builder pattern, its immediately recognizable, it has fluid discovery, and seems to be a pattern for the ages :)

I really don't like the idea of errors passing through the compiler if they can be avoided (huge point against with-er api)

I do like the DSL-like API, but it is not as easily discoverable as Builder

I have a strong preference for the ApolloCall style it is more consistent with other fluent apis in Kotlin like Collection chaining (.map.flatmap.fold etc)

@BoD
Copy link
Contributor

BoD commented Oct 7, 2021

I can see another small pro of the with approach: users of the lib can easily add their own withXyz() extensions, and their usage will blend nicely with the built-in ones. Not sure if the same can be said of the builder or DSL approaches.

But overall I still tend to agree with you that builders are probably the best compromise (the familiarity argument is a strong one for me).

@martinbonnin
Copy link
Contributor Author

I can see another small pro of the with approach: users of the lib can easily add their own withXyz() extensions, and their usage will blend nicely with the built-in ones. Not sure if the same can be said of the builder or DSL approaches.

I think we can support use cases like this and Builders with things like:

fun ApolloClient.Builder.userDefined(param: Param) {
  //
}

We'll need to support extensions in all cases for the normalized cache since it is completely independent from apollo-runtime.

@BoD
Copy link
Contributor

BoD commented Oct 19, 2021

The APIs have been migrated to Builders in #3404.
(Note: the previous constructors + With-ers API has been kept as @Deprecated temporarily, to ease the transtion. They will be removed before v3.0 stable is released).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants