Update ApolloClient and ApolloRequest APIs from With-ers to Builders #3404

BoD · 2021-10-11T16:10:53Z

Related to #3301, this adds a proposal to implement a Buildersbased API instead of the current With-ers based on on the 3.x branch.

design-docs/Builders.md

BoD · 2021-10-13T17:01:23Z

This PR is easier to read in a few steps:

Make ApolloClient's API Builder based instead of With-er based (8126eaa and f68807b)
Make ApolloRequest's API Builder based instead of With-er based (a81a3ba)
Split ExecutionParameters in 2 interfaces: read-only and mutable (d1eefb8 and 84deb62)
Rename all "with" extensions for consistency (59218b1)
Add a temporary "compatibility layer" keeping the previous With-ers API available, to ease the transition (c2424fd)

What's left:

Update documentation and upgrade guide
Maybe some unit tests for the Builders

apollo-api/src/commonMain/kotlin/com/apollographql/apollo3/api/ApolloRequest.kt

...he/src/commonMain/kotlin/com/apollographql/apollo3/cache/normalized/ClientCacheExtensions.kt

...monMain/kotlin/com/apollographql/apollo3/cache/normalized/internal/ApolloCacheInterceptor.kt

apollo-runtime/src/commonMain/kotlin/com/apollographql/apollo3/ApolloClient.kt

martinbonnin · 2021-10-14T09:40:26Z

A few comments but looks good overall, Thanks!

For the HasExecutionContext, we might want to make everything val instead of fun

ApolloClient.sendApqExtensions() => val ApolloClient.sendApqExtensions
ApolloRequest.httpHeaders() => val ApolloClient.httpHeaders
etc...

Especially for properties that start with a verb, it makes it explicit that it's only a getter? Any thoughts?

BoD · 2021-10-14T11:40:27Z

A few comments but looks good overall, Thanks!

🙏

For the HasExecutionContext, we might want to make everything val instead of fun

ApolloClient.sendApqExtensions() => val ApolloClient.sendApqExtensions

ApolloRequest.httpHeaders() => val ApolloClient.httpHeaders

etc...

Especially for properties that start with a verb, it makes it explicit that it's only a getter? Any thoughts?

Yes, good idea! I first thought of renaming them getXyz, but making them vals instead is better and clear IMO.

apollo-api/src/commonMain/kotlin/com/apollographql/apollo3/api/ApolloRequest.kt

martinbonnin · 2021-10-14T16:12:05Z

apollo-http-cache/src/main/kotlin/com/apollographql/apollo3/cache/http/HttpCacheExtensions.kt

+  return networkTransport(networkTransport.copy(
+          engine = CachingHttpEngine(
+              directory = directory,
+              maxSize = maxSize,
+              delegate = networkTransport.engine
+          )


Random thoughts: should we go Builders all the way down and convert HttpNetworkTransport to Builders as well?

Not completely sure. Maybe we could come up with a rule of thumb like: if there are more than n possible parameters, let's have a builder, otherwise a regular constructor is enough?

There are 2 strong arguments for Builders everywhere though:

consistency

java interop

Also add precision about splitting ExecutionParameters and adding a toBuilder method.

…PI available, to ease the transition

…for clarity abnd consistency

BoD · 2021-10-15T13:00:12Z

(Another PR with further doc updates following this change will come later.)

BoD added 🔍 Type: Investigation 2021-10 labels Oct 11, 2021

BoD requested review from martinbonnin, sav007 and tasomaniac as code owners October 11, 2021 16:10

BoD assigned BoD and martinbonnin and unassigned BoD Oct 11, 2021