Abstract NetworkService functionality #5
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tylermilner
reviewed
Jan 27, 2018
There was a problem hiding this comment.
@wmcginty looks good as well. Sync w/ 'master' and merge whenever you can.
| @@ -46,7 +46,7 @@ extension BackendService: BackendServiceProtocol { | |||
| networkService.cancelTask(for: request) | |||
| } | |||
|
|
|||
| private func handleResponseData<T: NetworkRequest, U>(_ data: Data, for request: T, completion: @escaping BackendServiceCompletion<U>) where T.ResponseType == U { | |||
| private func handleResponseData<T: NetworkRequest>(_ data: Data, for request: T, completion: @escaping BackendServiceCompletion<T.ResponseType>) { | |||
There was a problem hiding this comment.
I'm digging the simplification!
| /// | ||
| /// - Parameter clientError: The error returned by the NetworkSessionDataTask. | ||
| /// - Returns: The outcome NetworkServiceResult dictated by the error. | ||
| public static func networkServiceFailure(for clientError: Error?) -> NetworkServiceFailure { |
There was a problem hiding this comment.
For consistency with networkServiceResult(for response:) below, would we want to return a NetworkServiceResult here too?
There was a problem hiding this comment.
We could, but on the use side I ended up then having to immediately unwrap the NetworkServiceResult into the underlying NetworkServiceFailure to use it so I felt like it was a waste in this case to wrap into a Result to simply unwrap immediately after.
Maybe as an addition we could add a third function to the helper that returns the Result?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I pulled some of the conversion functionality out of NetworkService and intro a NetworkServiceStruct helper - this allows for a simpler approach to creating a delegated network stack. Because of the wide variety of implementation concerns, the framework can't really provide a way to store and execute
'completion handlers' using the URLSessionDelegate (and sub-protocols).
Because of this, it is simplest to bypass the majority of BackendService and NetworkService's execution functions and use URLSession directly. It is still desirable to use Hyperspace's error and result handling however, and breaking these functions out of NetworkService itself allows us to do that.