-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a way to get progress of uploading. #234
Add a way to get progress of uploading. #234
Conversation
While the upload is in progress, `progressHandler` closure is periodically called. Therefore you can get information of uploading about `didSendBodyData` `totalBytesSent` `totalBytesExpectedToSend`. These parameters are defined by `urlSession(_:task:didSendBodyData:totalBytesSent:totalBytesExpectedToSend:)` in `URLSessionTaskDelegate`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the great contribution 👍
Since it introduces tiny breaking change in SessionAdapter protocol, I would like to merge this into develop/4.0
. I left 1 small note about naming on your commit, please check it 🙏
Sources/APIKit/Session.swift
Outdated
@@ -37,8 +37,8 @@ open class Session { | |||
/// - parameter handler: The closure that receives result of the request. | |||
/// - returns: The new session task. | |||
@discardableResult | |||
open class func send<Request: APIKit.Request>(_ request: Request, callbackQueue: CallbackQueue? = nil, handler: @escaping (Result<Request.Response, SessionTaskError>) -> Void = { _ in }) -> SessionTask? { | |||
return shared.send(request, callbackQueue: callbackQueue, handler: handler) | |||
open class func send<Request: APIKit.Request>(_ request: Request, callbackQueue: CallbackQueue? = nil, progressHandler: @escaping (Int64, Int64, Int64) -> Void = { _ in }, handler: @escaping (Result<Request.Response, SessionTaskError>) -> Void = { _ in }) -> SessionTask? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think handler
should be renamed as completionHandler
so that users can understand the difference between progressHandler
and completionHandler
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
I agree with you on testing the progress delegate method. I understand that there're many limitations on NSURLProtocol, and I think your decision is very reasonable. Thank you for searching and introducing the solution! |
💯 |
thank you for reviewing!! 😄 |
See: #186
Introduction
Add
progressHandler(bytesSent:totalBytesSent:totalBytesExpectedToSend:)
parameter inSession.send
for getting progress of uploading.While the upload is in progress,
progressHandler
closure inSession.send
is periodically called.Therefore you can get information of uploading about
didSendBodyData
totalBytesSent
totalBytesExpectedToSend
.These parameters are defined by
urlSession(_:task:didSendBodyData:totalBytesSent:totalBytesExpectedToSend:)
in URLSessionTaskDelegate.Example
Suffering
In unit test about
urlSession(_:task:didSendBodyData:totalBytesSent:totalBytesExpectedToSend:)
delegate method in URLSessionTaskDelegate, unwillingly sending an real HTTP request...I couldn't stub because this delegate method will never be called when you stub the request using subclass of URLProtocol.
https://developer.apple.com/library/content/samplecode/CustomHTTPProtocol/Listings/Read_Me_About_CustomHTTPProtocol_txt.html
So sorry about making your tests higher the likelihood fail randomly 😢
After long consideration, I came up with a better way to stable test.
That way is building mocking webserver in local for testing HTTP clients.
This solution is used by okhttp (HTTP client library for java).
https://github.com/square/okhttp/tree/master/mockwebserver
But I don't think that it is a serious problem.
Therefore I don't implement but only introduce 😉