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
Flatten enums & parameters into extended Task type #1147
Conversation
SwiftLint found issuesWarnings
Errors
Generated by 🚫 Danger |
I have just integrated my fork into the project where I'm using the Here's how my target now looks: import Foundation
import Moya
enum MyTarget {
case createUser(CreateUserRequest)
case signIn(SignInRequest)
}
// MARK: - TargetType Protocol Implementation
extension MyTarget: TargetType {
var baseURL: URL {
return URL(string: "https://example.com/api/v1/")!
}
var path: String {
switch self {
case .createUser:
return "user"
case .signIn:
return "session"
}
}
var method: Moya.Method {
return .post
}
var sampleData: Data {
return Data()
}
var task: Task {
switch self {
case .createUser(let createUserRequest):
let requestData = encode(createUserRequest)
return .request(.data(requestData))
case .signIn(let signInRequest):
let requestData = encode(signInRequest)
return .request(.data(requestData))
}
}
}
// MARK: - Helper Methods
extension Tm5Target {
private func encode<T: Encodable>(_ encodable: T) -> Data {
do {
return try JSONEncoder().encode(encodable)
} catch {
fatalError("Could not encode '\(type(of: encodable))' using JSONEncoder. Error: \(error)")
}
}
} It builds fine and I think it should work (haven't tested yet). But this is how the usage might look now. Just to have something to review. |
I will go ahead and say we might not want to add it to 9.0.0 release since we have many breaking changes already (and few big ones). To make transition smoother we may just hold onto this one till after the release. This is a really big change and a great amount of work from @Dschee, so it would be awesome to get some feedback from @Moya/contributors in the meanwhile. This PR helps with a lot of undefined behavior but from the user perspective it might look messy. We may need to think about this one a little bit more: return .request(.parameters(["test1": "test", "test2": "test"])) Looks kinda uncomfortable at first glance. And especially because it is the default use-case, we may think of a cleaner solution. Would one enum instead of 3 help? Not sure. Opinions? |
I looked this over and it seems to make sense to me. I agree with @sunshinejr that it gets a lot more verbose at the call site... definitely not great. I'm wondering if instead of associating the new |
@ashfurrow Sure, why not. Think that makes sense. Just rebased onto the current master and added the requested change. The task computed property example from above now looks like this: var task: Task {
switch self {
case .createUser(let createUserRequest):
return .requestData(encode(createUserRequest))
case .signIn(let signInRequest):
return .requestData(encode(signInRequest))
}
} |
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 makes a lot of sense, good work 👍 What do you think @sunshinejr ?
Shall I flatten the |
Any preferences on how to deal with the exceptions thrown in places like this: do {
preparedRequest = try parameterEncoding.encode(preparedRequest, with: parameters)
} catch {
// TODO: Add exception handling here
} I think, I could deal with the other TODO steps left myself, but I'm not sure about that one. |
Sources/Moya/TargetType.swift
Outdated
case requestData(Data) | ||
|
||
/// A requests body set with parameters and encoding. | ||
case requestEncoded(parameters: [String: Any], encoding: ParameterEncoding) |
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.
maybe request
/requestParameters
? or requestWithParameters
? I feel like encoded
is not as a user-friendly name as parameters
😄 requestWithParameters
reminds me of Swift 2.0 though 😞
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've renamed to requestParameters
, although I also considered requestParameterEncoded
(tell me if you like that more, otherwise I'll keep it).
Sources/Moya/TargetType.swift
Outdated
|
||
/// The method used for parameter encoding. | ||
var parameterEncoding: ParameterEncoding { get } | ||
/// The default parameterEncoding for the `.encoded` RequestDataType case. |
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 feel like we don't actually need to add this to the core. We do not use it anywhere, and this can be easily added to the migration guide as a best practice to use. Or better yet, make an extension to ParameterEncoding
:
extension ParameterEncoding. {
static var default: JSONEncoding {
return JSONEncoding.default
}
}
This way it would be easily used in task:
return .request(parameters: [:], encoding: .default)
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.
Did as suggested. Two variations though. One: I used ParameterEncoding
as the return type isntead of JSONEncoding
. Two: I had to backtick default
as I saw an error (in Xcode 9 beta 3).
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.
Actually the extension didn't work correctly. I received the following error in Xcode 8.3:
Static member 'default' cannot be used on protocol metatype 'ParameterEncoding.Protocol'
I'm removing it now ... I think JSONEncoding.default
and URLEncoding.default
should be fine enough for now.
cancellableToken.innerCancellable = self.sendRequest(target, request: preparedRequest, queue: queue, progress: progress, completion: networkCompletion) | ||
|
||
case let .requestEncoded(parameters: parameters, encoding: parameterEncoding): | ||
do { |
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.
Usually this is done in an Endpoint
, take a look here. If we cannot encode parameters, we throw a MoyaError.requestMapping(_:)
error to the user.
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.
Sorry, I don't really understand how I could throw a MoyaError.requestMapping
from within the performNetworking
closure, also I have no idea how to move the code to the endpoint. How can I switch on the target.task
from within the Endpoint class? Need some help with this ...
I left you comments around most issues @Dschee. For flattening I agree we would need to flat upload & download types as well. I think we might do it in the same PR since naming convention in this one would be similar. Anyhow, I like the direction that this PR has taken. Keep it up :) |
Sorry for letting you wait for two weeks, I've been on vacation this entire time. Now that I'm back I've had a look onto your feedback @sunshinejr and commented on them (two of three I fixed). Also I have flattened the upload and download types into the Other than that, is there anything that speaks against merging this except for the TODOs on the initial Post that are still open? If not, then I would proceed implementing the TODOs so we can merge this soon. |
Hey @Dschee, just a heads-up. I'll try to get to it ASAP and edit this comment. Thank you for the constant work on this one. 👍 |
Okay, I have now worked through my TODOs. Except for the exception handling, I think this is now finished. I've removed the Btw, when I updated the docs, I've seen that some examples are outdated and I didn't fix that (although my code change has to do with it) cause it is using an old syntax of Alamofire. Specifically I'm talking about https://github.com/Moya/Moya/blob/master/docs/Examples/ArrayAsRootContainer.md We should think about removing or updating it in a different PR / Issue. |
Just rebased onto the current master so things are updated (and tests can pass, hopefully). |
httpHeaderFields: [String: String]? = nil) { | ||
|
||
self.url = url | ||
self.sampleResponseClosure = sampleResponseClosure | ||
self.method = method | ||
self.parameters = parameters |
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 that we can't just remove option to update parameters
/parameterEncoding
in the Endpoint closure. Yes, we don't have it as separate properties, but we can replace it with task
- it would be even more customizable.
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.
Ok, let me try ...
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 I got this now. Note that there's no convenience .addingParameters
or something. Don't know if we can provide a replacement with the task enum. We can't append all enum cases with each other (e.g. Data with Data), can we?
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.
We probably can't, yeah. But I think just the fact that you can change the task yourself in the endpointClosure
is enough, though.
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.
Sounds good. 👍
Sources/Moya/Endpoint.swift
Outdated
@@ -99,7 +69,7 @@ extension Endpoint { | |||
request.httpMethod = method.rawValue | |||
request.allHTTPHeaderFields = httpHeaderFields | |||
|
|||
return try? parameterEncoding.encode(request, with: parameters) | |||
return request |
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 is a place where you would do a switch and prepare the request based on a task. We don't really want to do it in the core file since preparing the request based on an Endpoint
is Endpoint
's responsibility. Another benefit is the fact that when we return nil, there is an error thrown in basic request mapping and we don't even have to do a request (source).
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.
Ok, I see. Let me try ...
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 I got this, too. Trying to clean up the internal MoyaProvider now ...
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.
Ok, that was easy. As you suggested, I'm now returning nil when an exception is thrown. This way I can even mark my last TODO as completed. Thanks for the feedback! 👍
Sources/Moya/TargetType.swift
Outdated
@@ -30,37 +24,42 @@ public protocol TargetType { | |||
} | |||
|
|||
public extension TargetType { | |||
var defaultParameterEncoding: ParameterEncoding { |
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 we can remove the defaultParameterEncoding
from our codebase - I meant that someone can implement it in their own codebase if its convenient for them.
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.
Uh, I forgot to remove it (thought I actually did that). No problem with that.
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 this.
Sources/Moya/TargetType.swift
Outdated
|
||
/// Represents a type of download task. | ||
public enum DownloadType { | ||
/// A requests body set with parameters and encoding. |
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.
"A requests body set with encoded parameters"
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.
Readme.md
Outdated
|
||
Please follow the appropriate guide below when **upgrading to a new major version** of Moya (e.g. 8.0 -> 9.0). | ||
|
||
### Upgrade from 8.x to 9.x |
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 feel like this have its own file in the docs.
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.
Sure, I've moved it to its own MigrationGuides.md
file within the docs
folder. I think we should consider linking to the file when doing major releases within the changelog & release notes.
/// Represents a type of upload task. | ||
public enum UploadType { | ||
/// Represents an HTTP task. | ||
public enum Task { |
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.
we also need to properly test encoding the request from an endpoint
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.
Can we do that in a subsequent PR so this one can be in the 9.0 release? Cause I won't have much time to work on this in the near future (I'm concentrating my free time onto this project and on Carthage/Carthage#1990 right now) and I feel like the logic of adding the task cases data to the request isn't overly complicated, so it should all work fine. Maybe except for some edge cases, but those would probably be related to the new cases (like .composite...
) anyways ... which nobody uses yet.
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.
Making this one into the 9.0.0 release would be cool, but we can't do that without testing it (this is one of the major things we changed in the whole core). We can do that in separate PR, but still I think merging this should happen with tests in it.
I understand that you may be busy now, though. If that's the case, let me know and maybe someone can help with the tests and additional feedback on the code review (and rebase in the near future). There is still work to do, unfortunately, but we are close.
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 have rebased and worked through some more feedback from @SD10. I'd be happy to work through any other feedback this weekend, but regarding tests it would be great to have help there. I don't think I'll have the time to set them up in addition to reacting to change requests ...
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 can help with tests, no problem with that 👍 I'll try to do it ASAP.
httpHeaderFields: [String: String]? = nil) { | ||
|
||
self.url = url | ||
self.sampleResponseClosure = sampleResponseClosure | ||
self.method = method | ||
self.parameters = parameters |
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.
We probably can't, yeah. But I think just the fact that you can change the task yourself in the endpointClosure
is enough, though.
Sources/Moya/Endpoint.swift
Outdated
request.httpBody = data | ||
|
||
case let .requestParameters(parameters: parameters, encoding: parameterEncoding): | ||
do { |
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.
we can just do returns in cases I think:
return try? parameterEncoding.encode(request, with: parameters)
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.
Good catch, updated the cases requestParameters
and downloadParameters
where this was possible. This didn't work in the composite cases though as they need to to multiple things with the request.
Worked through your comments again and rebased. Are you planning to add this to the current release after the alpha is out? Would be awesome to see this in the next version. :) |
Thanks, @Dschee! I'd love to have it, but we are still without tests and I'm not the only one to decide such things. Would love to get some feedback on it from @Moya/contributors, since it is almost finished now. Also, sorry that I didn't mentioned it earlier, but would you rebase it to |
Thanks @sunshinejr, I've worked through your comments and rebased onto the |
Codecov Report
@@ Coverage Diff @@
## 9.0.0-dev #1147 +/- ##
=============================================
- Coverage 79.53% 78.48% -1.06%
=============================================
Files 23 23
Lines 738 739 +1
=============================================
- Hits 587 580 -7
- Misses 151 159 +8
Continue to review full report at Codecov.
|
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 this should be ready to go after testing. The basic documentation updates look good too. I'm a bit busy right now but can help with any missing documentation after the merge.
@Dschee Thank you for all your hard work and re-iterating on all the requested changes 🍻
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.
Let's merge it and test it both manually and automatically to make sure we didn't miss anything 👍 Thank you so much for your work @Dschee and everyone involved, really good work around here, 100+ comments, 30+ commits 😮
Amazing work everyone, @Dschee especially! 💪 |
Thank you everyone for reviewing and going all the way with me. You're the best! 💪 👏 👍 |
Thank you for your work on this, however I'm having issues getting it to work. I looked through this thread hoping to figure it out, but unfortunately I've come up empty. Is there an example or documentation on how to get .requestData to work in my Task? |
Hey @T1ASH. If I understand your question correctly, you can do it as follows: enum GitHub {
case zen
}
extension GitHub: TargetType {
...
var task: Task {
let data = "test data".data(using: .utf8)
return .requestData(data)
}
...
} Where, of course, your Let me know if this is what you wanted or I've completely missed 😄 |
@sunshinejr I Edited your reply to be |
Damn it! Thanks for the great catch, though, @pedrovereza! |
How can I use the encode extension? Was it added to the library or should I simple add it myself? |
This solution was dicussed starting here:
#1135 (comment)
Specifically this PR does the following:
parameters
andparameterEncoding
from everywhereTask
with different types of request data (e.g.data
&compositeEncoded
)DownloadType
andUploadType
enums into theTask
typeTODO:
comments (exception handling) appropriatelyThis solves the most apparent part of #1135 and it probably also solves #314.