Use the streaming HttpEngine by default on Apple #5671
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
| if (nsUrlSession == null) nsUrlSession = NSURLSession.sessionWithConfiguration( | ||
| configuration = NSURLSessionConfiguration.defaultSessionConfiguration(), | ||
| delegate = delegate, | ||
| delegateQueue = null | ||
| ) | ||
| return nsUrlSession!!.dataTaskWithRequest(request) |
There was a problem hiding this comment.
This is prone to race conditions and also surprising because delegate would only be taken into account for the first call. What about
fun interface DataTaskFactory {
fun dataTask(request: NSURLRequest): NSURLSessionDataTask
}
class DefaultHttpEngine(
private val timeoutMillis: Long = 60_000,
private val dataTaskFactory: (NSURLSessionDataDelegateProtocol) -> DataTaskFactory,
) : HttpEngine {There was a problem hiding this comment.
Also more general comment about fun interface. We don't have many in the codebase and I think I'd prefer not adding too many. Regular interfaces are probably easier to evolve if we need to
There was a problem hiding this comment.
Ah but wait, we need to control the delegate so looks like we need a factory of factory instead 🙈 . I've edited my initial comment
| */ | ||
| @ApolloExperimental | ||
| class StreamingNSURLSessionHttpEngine( | ||
| actual class DefaultHttpEngine( |
There was a problem hiding this comment.
Can you remove the expect/actual classes to use top level constructor functions instead? (Can be made in a separate PR)
There was a problem hiding this comment.
Will do it in the next PR (also to implement Closeable to replace dispose())
| interface DataTaskFactory { | ||
| fun dataTask(request: NSURLRequest): NSURLSessionDataTask | ||
| } | ||
|
|
||
| private class DefaultDataTaskFactory(delegate: NSURLSessionDataDelegateProtocol) : DataTaskFactory { | ||
| private val nsUrlSession = NSURLSession.sessionWithConfiguration( | ||
| configuration = NSURLSessionConfiguration.defaultSessionConfiguration(), | ||
| delegate = delegate, | ||
| delegateQueue = null | ||
| ) | ||
|
|
||
| override fun dataTask(request: NSURLRequest): NSURLSessionDataTask { | ||
| return nsUrlSession.dataTaskWithRequest(request) | ||
| } | ||
| } |
There was a problem hiding this comment.
Thinking about this more, do we even need DataTaskFactory? Can't we "just" have
actual class DefaultHttpEngine : HttpEngine {
constructor(
timeoutMillis: Long = 60_000,
configuration: NSURLSessionConfiguration?
) {
What is the use case for changing anythign else than the configuration?
There was a problem hiding this comment.
I think we want to allow people to have a global NSURLSession for their app, like we do on the JVM with constructors that accept a OkHttpClient or Call.Factory.
But honestly it's hard to tell if it's actually useful without being an iOS dev 😅
There was a problem hiding this comment.
I think we want to allow people to have a global NSURLSession for their app, like we do on the JVM with constructors that accept a OkHttpClient or Call.Factory.
Given we force the delegate, I don't think the NSURLSession can be reused by any other parts of the app so we might as well own it (also we're talking about closing it ourselves so if it is shared we def. don't want to close it and we'd have to refcount and what not)
Also looks like the iOS client allocates its own URLsession: https://github.com/apollographql/apollo-ios/blob/b434fb904b4695bcc84e83ca1cac132321bf5435/Sources/Apollo/URLSessionClient.swift#L64
There was a problem hiding this comment.
Good points, yeah that sounds about right. Simply passing a NSURLSessionConfiguration is definitely a welcome simplification 👍
Resolves #5659