Add no-internet connection monitoring#52
Conversation
Apple has eliminated usage of the term "localWiFi" for reachability. For more details: https://developer.apple.com/library/archive/samplecode/Reachability/Listings/ReadMe_md.html
| ) | ||
| } | ||
|
|
||
| final class ClientTaskExecutor: NSObject { |
There was a problem hiding this comment.
Do we really need another Executer? The Client is now working Client.post -> Executer -> Executer. Maybe it is possible to integrate the reachability into the RequestExecuter by composition to reduce the complexity?
There was a problem hiding this comment.
Eventually you forget UploadExecuter and DownloadExecuter. I just didn't want to add the same code in those components.
| public final class Client { | ||
| public typealias RequestCompletion<ResponseType> = (HTTPURLResponse?, Result<ResponseType, Error>) -> Void | ||
|
|
||
| internal enum Task { |
There was a problem hiding this comment.
Do we really need the additional task abstraction?
| } | ||
|
|
||
| final class ClientTaskExecutor: NSObject { | ||
| internal static let `default` = ClientTaskExecutor() |
There was a problem hiding this comment.
Type definitions should be on the left Side of the declaration.
| do { | ||
| try self.reachabilityManager?.startListening(on: .main) { _ in } | ||
| } catch { | ||
| NSLog("[WARNING] Faild to start network monitor. Error: \(error)") |
There was a problem hiding this comment.
We do have a Logger implementation in the framework.
| super.init() | ||
|
|
||
| do { | ||
| try self.reachabilityManager?.startListening(on: .main) { _ in } |
There was a problem hiding this comment.
Whats happening in the completion? Do we need a log here as well?
There was a problem hiding this comment.
The closure is not about completion but callback on state update. We do not really observe the reachability all the time. In this implementation we just check that a connection can be established before we perform a network request.
| } | ||
|
|
||
| switch (executor, task) { | ||
| case (is RequestExecutor, let .dataTask(request, completionHandler)) : |
There was a problem hiding this comment.
For casting in switch cases please take a look at the example of the swift documentation:
switch thing {
case 0 as Int:
print("zero as an Int")
case 0 as Double:
print("zero as a Double")
case let someInt as Int:
print("an integer value of \(someInt)")
case let someDouble as Double where someDouble > 0:
print("a positive double value of \(someDouble)")
case is Double:
print("some other double value that I don't want to print")
case let someString as String:
print("a string value of \"\(someString)\"")
case let (x, y) as (Double, Double):
print("an (x, y) point at \(x), \(y)")
case let movie as Movie:
print("a movie called \(movie.name), dir. \(movie.director)")
case let stringConverter as (String) -> String:
print(stringConverter("Michael"))
default:
print("something else")
}source: https://docs.swift.org/swift-book/LanguageGuide/TypeCasting.html
| throw ClientTaskError.connectionUnavailable | ||
| } | ||
|
|
||
| switch (executor, task) { |
There was a problem hiding this comment.
If we use composition and we add this functionality to the request executer we do not need the cast.
| /// if you can connect to a particular host, | ||
| /// only that an interface is available that might allow a connection, | ||
| /// and whether that interface is the WWAN. | ||
| open class NetworkReachabilityManager: NetworkReachabilityMonitor { |
There was a problem hiding this comment.
Please add source links and description to the implementations which you used as "inspiration".
| /// network reachability status. | ||
| public typealias NetworkReachabilityStateCallback = (NetworkReachabilityState) -> Void | ||
|
|
||
| public protocol NetworkReachabilityMonitor { |
There was a problem hiding this comment.
Maybe you could rename the protocol to NetworkReachabilityMonitoring
| completion: completion | ||
| ) | ||
| } | ||
| return try taskExecutor.perform(task, on: requestExecutor) |
There was a problem hiding this comment.
I'm not sure if this was the intention of @SimonNumberTwo that the client is throwing an error if there is no internet connection. I guess he meant that the completion of the function should be called with an error.
@SimonNumberTwo can you give some more details about the usage in our apps? What should be the expected behavior?
There was a problem hiding this comment.
Yes exactly. I would prefer to have an error case for no internet and let the app itself using Jetworking decide what to do with it rather than implicitly throwing an error.
|
@jjorn What's the actual state of this PR? |
3 participants
Description
This PR implements a monitoring concept for availalbe internet connection.
Related Issue
This PR should also solve #43
Changes:
SystemConfigurationframeworkNetworkReachabilityManageracting as a monitor of current internet connectionClient.Taskas a wrapper for the client operations -request,uploadanddownloadClientTaskExecutorincluding network connection monitoringNotes
The feature of reachability has been built on
SystemConfigurationframework. As an alternative, we can also useNWPathMonitorinNetworkframework which is available from iOS 12.0.