Add NormalizedCacheFactory.close() #67
Conversation
| expect fun SqlNormalizedCacheFactory(driver: SqlDriver): NormalizedCacheFactory | ||
| fun SqlNormalizedCacheFactory(driver: SqlDriver): NormalizedCacheFactory = SqlNormalizedCacheFactory(driver, false) | ||
|
|
||
| internal fun SqlNormalizedCacheFactory(driver: SqlDriver, manageDriver: Boolean): NormalizedCacheFactory = |
There was a problem hiding this comment.
The manageDriver here seems inconsistent. I would avoid having to manually define ownership. There are 2 cases:
- We want the connection to be shared accross multiple clients => expose it as a public property so the user can retrieve and close it manually if they want to. (see OkHttpClient.dispatcher.executorService.shutdown()).
- We want ApolloClient to own the connection => close it systematically.
Option 1. is not breaking current behaviour. On the other hand, 2. is probably what most users expect by default. At the end of the day, it all boils down to whether there's a good case for sharing a connection. I would intuitively say "no" but haven't thought that completely through.
There was a problem hiding this comment.
I like 1! If we expose the driver, there's no need for a close.
About 2, yeah currently it's possible to pass the same factory to multiple clients, which should probably be forbidden?
There was a problem hiding this comment.
it's possible to pass the same factory to multiple clients, which should probably be forbidden?
If we forbid sharing a connection between multiple clients, we might as well close the connection systematically when the client is closed. At first sight I would say this is what the majority of users expect.
There was a problem hiding this comment.
I forgot that ApolloClient.close has no knowledge of ApolloStore 😅 To make this happen we could add a ApolloStore.manage(Closeable)ApolloClient.manage(Closeable)
There was a problem hiding this comment.
Either that or ApolloInterceptor.close() (HttpInterceptor has something like this already for batching IIRC)
|
Closing for now until we figure out a satisfying mechanism to handle closing |
Related to #66
SqlNormalizedCacheFactory.close()will callclose()on theSqlDriverit creates - if you pass your own driver, it won't close it.