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
Multipart upload #483
Multipart upload #483
Conversation
25155cf
to
eb4623d
Compare
Hey @ashfurrow! I improved the API a little bit. I created a |
Cool! Want to email me at ash@ashfurrow.com to schedule a time that works? |
373e284
to
7deaa41
Compare
Cool, this looks awesome. I think we have the following TODOs to get this wrapped up:
I'll try to take a look at this this weekend. |
@leonereveel: I just needed multipart upload and started using your fork. Once I changed my I tried experimenting with a slightly different API that I think is simpler. What do you guys think? |
@@ -4,6 +4,28 @@ import Result | |||
/// Closure to be executed when a request has completed. | |||
public typealias Completion = (result: Result<Moya.Response, Moya.Error>) -> () | |||
|
|||
public typealias ProgressBlock = (progress:ProgressResponse) -> Void |
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.
For consistency with the rest of the code, I think we should add a space after :
(progress: ProgressResponse
). Doesn't have to be part of this PR, we could clean up afterwards. 😃
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.
Agreed!
Definitely good stuff in this PR - and I like @hirad's changes in his experimental branch, FWIW. |
Cool guys! Let me know if you need any contribution. @hirad The way you did works very well and make things more simple. I'll try it out here. |
let plugins = self.plugins | ||
|
||
let multipartFormData = { (form:RequestMultipartFormData) -> Void in | ||
let params = decodeHTTPBody(request.HTTPBody) |
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.
This part is a little iffy to me. What happens if the parameter encoding wasn't .URL
? Then the value in params
here will be malformed. I'm not sure what'd be the best way to handle encoding of additional parameters in a multipart upload.
One option might be wrapping Alamofire's ParameterEncoding
and providing our own encoding option for multipart?
In any case, I think decoding the already-encoded value is problematic. We have access to all the raw values anyway (through target.parameters
or better yet self.endpoint(target).parameters
). I think we should use that directly.
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.
Agreed!
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.
@hirad I merged your pull request and added the parameter encoding using Alamofire's URL encoding. I don't think we need to support other encoding types on multipart form.
Hey @ashfurrow! @hirad added RxCocoa support. =)
Current coverage is 65.02%
|
@leonereveel this looks great! What's left before we can use it? |
@AndrewSB I need to do some tests, which I will have time next week. Community review and merge. |
I'm def happy to review this, I'm excited to (finally!) get uploads into Moya. Please ping me ( |
344e37f
to
9f51dd6
Compare
SwiftLint found issues\n\n#### Warnings
Errors
Generated by 🚫 danger |
^ Looks like Swift-lint is running once per target, might want to have an option there |
I merged from master and improved the code a little bit. Can anyone help me to test it? @colinta? |
Cool! I will not be able to today, but definitely will do tomorrow. |
If define |
No it's still not as simple as that, but I'm learning more about what's going on. So because func request() -> String {
var cancellable = "foo"
inOneSecond {
cancellable = "bar"
}
return cancellable
}
request() // always "foo" BUT, I see that I propose this: for one, I agree that it seems like Something like this: // change to a class, for reference semantics
internal class CancellableWrapper: Cancellable {
// calling cancel() on this initial instance will just update the canceled value
internal var innerCancellable: Cancellable = SimpleCancellable()
// renamed 'isCancelled' for consistency
var canceled: Bool { return innerCancellable.canceled ?? false }
internal func cancel() {
innerCancellable.cancel()
}
}
internal class SimpleCancellable: Cancellable {
var canceled = false
func cancel() {
canceled = true
}
} |
I'm creating #530 to address this issue - and I've got a branch that combines #530 and #483 here: https://github.com/colinta/Moya/tree/multipart-after-refactor |
Good solution @colinta! 👍 I'm looking forward to see everything merge into master branch (upload and this refactoring). I'm glad to see that this discussion solved another possible bug. |
@ashfurrow @colinta If you need any help to document this, let me know. I can do that this weekend. |
This is fantastic work and a great example of open source collaboration. Thanks everyone! Wondering about the new property in the TargetType protocol: |
I tried that @ashfurrow and it override user's implementation. One of my commits was only to remove the extension. |
💯 |
…actor-2 * colinta/refactor/cancellables: new specs: tests Cancellable and cancel() logic fixes to specs around Cancellable changes renamed 'canceled' to 'cancelled' for consistency w/ protocol names refactor Cancellables: use class semantics, assign default 'innerCancellable' a non-cancelled request spec [Danger] Updates SwiftLint plugin. add Changelog entry remove NetworkCurlLogger.swift from .xcodeproj
Brings in the updates from the Cancellable refactors
Fixed multipart-parameters encoding. I didn't know that this must be encoded differently. |
Tests pass, not sure why Danger failed but I'm gonna merge 💯 |
Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions. |
Sweet! I agree about the collaboration. Great feature, and tons of people working on it - awesome!
|
This the same implementation as #481 but is based on master branch.
I need to support file upload using Moya and after look at #114 discussion, I decided to implement my own solution.
I just added a MultipartTargetType protocol and made some extensions to MoyaProvider where the Target matches the new type.
Here is an example:
requestWithProgress
method returns aProgressResponse
that contains the progress and the response result when the http call is completed. Use the methodfilterCompleted()
to get the actualResponse
andfilterProgress()
to get the progress of the Upload.I had some trouble to compile and test the project. I can create some tests and add to this PR if some one help me with that.
Thank you and enjoy!