Moya: Native support for Encodable types in Swift 4

[Jeehut]

I'm currently writing an app to be released after the official release of iOS 11 that'll only support iOS 11 onwards. So naturally I'm using the Xcode 9 betas and I've also decided to write code in Swift 4 from the beginning. What I try right now is to combine the great new possibilities with the Encodable and Decodable protocols with Moya. While there's #1118 for adding native support for the Decodable protocol, I'm currently struggling with the Encodable protocol.

My idea was to encapsulate the request parameters into it's own type (they are completely different than the response data, so it's Encodable only):

struct SignInRequest: Encodable {
    // MARK: - Sub Types
    struct AppInfo: Encodable {
        let clientName: String
        let clientVersion: String
        let platformName: String
        let platformVersion: String
        let pushToken: String?
    }

    // MARK: - Stored Instance Properties
    let appInfo: AppInfo
    let username: String
    let password: String
}

My API target looks like this:

enum MyTarget {
    case signIn(SignInRequest)
    case profile(String) // example for classical approach
}

Now when making MyTarget conform to TargetType I have to provide the parameters and parameterEncoding computed properties. But the parameters have a return type of [String : Any]? which I can't create using JSONEncoder and also the API of ParameterEncoding is not flexible enough to write an own type that would conform to it and use the new Encodable inference from Swift 4.

What I need would be something like this as a _replacement_ of parameters and parameterEncoding:

var requestData: RequestData? {
   switch self {
   case .signIn(let request):
       return .jsonEncodable(request)

   case .profile(let id):
       return .parameterEncoding(["id": id], URLEncoding.default)
   }
}

For this to work, we would need to add a RequestData type like so:

enum RequestData {
    case jsonEncodable(Encodable)
    case parameterEncoding([String: Any], ParameterEncoding?) // a default parameter encoding could be provided in the TargetType, too (with an empty default implementation)
}

What do you think? I think we should definitely support Encodable types as first class citizens as many people will use them instead of mappers. The only question is, how can we make the TargetType flexible enough to support them?

Any ideas around how I could get this work with Moya 8.0 would be appreciated as well (for the time until this issue is fixed). Or did I miss something and can use Encodable already somehow in a good way? Cause AFAIK Moya is intended to be as flexible as possible.

[BasThomas]

First of all, thanks for looking into this! It seems like JSONEncoder returns Data and not [String: Any], so I am not sure how we could use the encoder for parameters in that case; we really need a [String: Any] for parameters.

Also, I am not quite sure what RequestData would do over parameters and parameterEncoding?Does it offer anything other than combining these two?

Keep us updated on your progress, we'd love to see the possibilities. You might want to consider forking Moya and don't be afraid of trying things out there. 😃

[Jeehut]

The fact that JSONEncoder returns Data ist exactly the reason why I've created RequestData. Its purpose is to allow both variants, the "classic" variant with parameters and parameterEncoding and using the JSONEncoder and its Data return type. With little changes elsewhere too this should allow us to use the above example to specify APIs.

I think the biggest drawback about this approach is the fact that it's a breaking change and everybody would need to use the requestData instead of parameters and parameterEncoding. Also note that it's not so easy any more to group same parameterEncoding cases together to just return once, but usually that should not be a problem when we add an option to define a defaultParameterEncoding.

I know I can fork this (although I could also create a branch within this repo), but I'd like to get some feedback first as we could tackle this problem in many different ways. Anybody else who needs this?

[mbuchetics]

@BasThomas why do you "really need a [String: Any] for parameters."? I see that it's the case right now (and also with Alamofire) but you could implement a new ParameterEncoding that takes a Data object instead of [String: Any] as its input.
I really think that the built-inEncodable and Decodable protocols are the way to go forward.

[Jeehut]

@mbuchetics I agree that Encodable is the way to go forward. But we also have to consider that people are using Moya in their released apps and migration should not be too hard. We should not force Moya consumers to use Encodable, there will still be valid reasons not to use that approach.

So, we can't simply drop the parameters without an alternative (IMHO), thus I'm still keeping them with my RequestData wrapper. What I try is to abstract away the return type of the params and making it flexible with my RequestData enum. There could be a different return type for each case, e.g. Data for .jsonEncoder and [String: Any] for Alamofires parameterEncoding. It just needs to be handled somewhere down in Moya. Data should be straightforward to handle though, as URLRequest has native support for setting the body via Data (which is probably also the reason why the .encode method of JSONEncoder has a return type of Data, I guess).

[Jeehut]

I'm going to implement my above suggestion then, as it seems I did not misunderstand the issue here. This way we can figure out if we can come up with an even better solution to this problem or keep my approach. I'm gonna try to post an initial PR until sometime tomorrow.

[mbuchetics]

I like your approach with the enum.

Another approach would be to keep the parameters as is and add an additional data property on TargetType. A new ParameterEncoding, let's call it DataEncoding could take the data and simply set it as HTTPBody whereas other encodings could behave the same way they are doing right now.
This would not break any existing code.

But I wouldn't worry to much about breaking existing apps if the migration is simple and easy to do. There were changes of this scope before (ParameterEncoding, task).

[Jeehut]

Thanks for the alternative. I've already thought about this solution. The problem with it is, that when having both type of API endpoints, the developer has to ensure that all cases of the TargetType are covered and split correctly between the two options. I think the risk that developers forget a case or specify two different types for a case is very high and would lead to some kind of error or unexpected behavior. That's why I introduced the enum, which is a little more code to write but prevents any such issues.

[mbuchetics]

You are correct with the added complexity.
However, I don't think yo necessarily need to split it up between the two options. There could be a case where you would want both, the parameters and the data, e.g. if you have a request that needs both, a URL query string and a HTTP body. So far, it always required workarounds such as https://github.com/Moya/Moya/issues/314
The ParameterEncoding could decide whether it uses parameters, data or both.

But as I said before, I also like your enum solution.

[Jeehut]

Hmm, I see your point now. I've actually used that workaround, too. 😄

So, let's think about this for a second. Until now we had to provide [String: Any]? and we could only specify one type of parameterEncoding. My approach does abstract away the type of [String: Any]? by using RequestData as the return type – but I'm still keeping the fact that you can only specify one.

What if we just changed the return type to [RequestData] so people could potentially return multiple of them? Does that make sense? Are there any other cases except for the URL params and body case where this would be useful? Cause if not, I tend to say that a return type of [RequestData] would be too confusing for many and would open the door for misuse cause people could put any number of things in there.

In my opinion we should rather cover the one exception case with special treatment rather than make the return type an array if there's no other useful case. (Although I'm not strongly opinionated on that.) But if there's another useful case that we can think of, I think it makes sense to implement it that way. What do you think?

Also what's the take of other @Moya/contributors on this issue and ideas on how to approach it? I think this is a core change and therefore should be validated by more contributors.

[Jeehut]

Here's an idea on how we could cover the one case with url params and body data with my approach:

enum RequestData {
    case jsonEncodable(encodable: Encodable)
    case parameterEncoding(parameters: [String: Any], parameterEncoding: ParameterEncoding)
    case urlAndBodyParameterEncoding(urlParameters: [String: Any], bodyParameters: [String: Any], bodyEncoding: ParameterEncoding)
}

The case urlAndBodyParameterEncoding would specify both URL parameters and body parameters. The body parameter encoding can be further specified (so people can choose between JSONEncoding, PropertyListEncoding or any of their custom encoding).

I think this is a better approach than to expect a return type of [RequestData] and works well with my approach explained on my initial post.

[mbuchetics]

I think the combination of URL query parameters and an Encodable would also
be useful.
Not sure if those rather obscure variants should be in the enum or if there
is a better way to handle those.

On 23 June 2017 at 16:16:49, Cihat Gündüz ([email protected]) wrote:

Here's an idea on how we could cover the one case with url params and body
data with my approach:

enum RequestData {
case jsonEncodable(encodable: Encodable)
case parameterEncoding(parameters: [String: Any], parameterEncoding: ParameterEncoding)
case urlAndBodyParameterEncoding(urlParameter: [String: Any], bodyParameters: [String: Any], bodyEncoding: ParameterEncoding)
}

The case urlAndBodyParameterEncoding would specify both URL parameters
and body parameters. The body parameter encoding can be further
specified (so people can choose between JSONEncoding, PropertyListEncoding
or any of their custom encoding).

I think this is a better approach than to expect a return type of
[RequestData].

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Moya/Moya/issues/1135#issuecomment-310677003, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAM6_XeB_Z613PyljaFNBJQ8uSkY13LOks5sG8jRgaJpZM4OCUcf
.

[Jeehut]

What is it that makes you think they‘re obscure? Their name? And wich ones, only the last one, right?

[Jeehut]

I've created an initial PR in #1136 to implement my approach. Please have a look – I'm not sure yet if it even works. Thus, your feedback is welcome!

[sunshinejr]

Hey guys. I must admit I didn't read the whole conversation, but I'd suggest that we wait a little bit with planning this one. We are still focusing on releasing 9.0.0 first. Additionally, we build Moya on top of Alamofire, and these guys could also implement some ParameterEncoding interop with Encodable & Decodable, thus making our life easier (they change ParameterEncoding a lot). Swift 4 has a long way to go and time may play on our side. Although if we don't get any new API, enum idea with some tweaks seems good for me :)

[mbuchetics]

I agree that it would make sense to have this supported via Alamofire and I am surprised that there is no discussion of this over there already.
Why do you think that Swift 4 has a long way to go? I think the new feature are all defined and most of them implemented, so I'd rather see them supported in Moya sooner than later. The transition to Swift 4 will be a lot quicker than with Swift 3.

[BasThomas]

Swift 4 will probably be released around September, which means we still have a two+ months to go, during which Swift will still be in development and will have some fixes and improvements.

Once it is released, the transition to Swift 4 will probably be pretty fast indeed, so I think we should aim to support Swift 4 sometime in September / October.

[Jeehut]

Why wait for this feature until September/October? The Encodable type will most likely not change at all as it was showed off during WWDC in detail without any hint on any possible changes that are planned to be made. I agree that any Alamofire-internal change would be welcome to cover this, but why not support this in a branch and further explore what else could be tackled by this approach, cause until now we are pretty dependent on Alamofires parameters solution, which was also the reason why #314 still isn‘t really solved.

If we really want to depend on Alamofire on this, we should at least get the opinion of its contributors on supporting Encodable before we decide that we just wait. Cause my gut feeling is that we might need to add some kind of enum into Moya anyways cause their solution might not be via the parameters and parameterEncoding, we just don‘t know and could wait for nothing in the end.

[BasThomas]

Earlier is better, and having a beta period would be great, so we can release a non-beta once Swift 4 is out of beta.

[sunshinejr]

Actually, we have some undefined behavior in Moya already, so maybe we can start implementing the changes you talk about right now (and then add Encodable later for Swift 4.0). Because there is a problem between task & parameters & parameterEncoding (especially in upload, parameters & parameterEncoding do nothing in it), maybe we can merge it and create a room for Encodable in the future? I imagine something like this (while removing parameters & parameterEncoding properties):

enum Task {
    case request(RequestType) // that's new
    case download(DownloadType)
    case upload(UploadType)
}

// this whole enum would be new
enum RequestType {
    case data(Data) // may be used with Encodable later or/and maybe we can add .encodable case too
    case parameters([String: Any])
    case encoded(parameters: [String: Any], encoding: ParameterEncoding)
    case composite(urlParameters: [String: Any], bodyParameters: [String: Any])
}

// expanded current DownloadType a little
enum DownloadType {
    case destination(DownloadDestination)
    case parameters(DownloadDestination, parameters: [String: Any])
    case encoded(DownloadDestination, parameters: [String: Any], encoding: ParameterEncoding)
}

// this could stay the same as it is
enum UploadType {
    case file(URL)
    case multipart([MultipartFormData])
}

This is just expanding our current Task API to cover more cases. We could also merge it into one enum making it kinda more easy to use (just make sure the most common use-case, request, would be easy to use). The problem would be with endpointClosure - what would adding the parameters & parameterEncoding mean for upload & uploadMultipart?

What do you think guys?

[Jeehut]

I'm in for the idea of introducing a more flexible parameter providing approach in general and using this to simplify the task along the way. That could even cover the usage of Encodable by default (without any specific changes for that). I'm not sure though what all the cases mean that you are providing. Specifically:

1. In the case of parameters([String: Any]) in RequestType – how are the params encoded, URL for GET and JSON for others by default? Or URL encoded always per default? Or something else?
2. What are all the cases in DownloadType? I'm guessing that destination(DownloadDestination) replaces the existing request(DownloadDestination)? What are the others then?
3. In DownloadType for the case case composite(urlParameters: [String: Any], bodyParameters: [String: Any]): How do we know the encoding to be used for encoding the body parameters?

Also, given we would tackle the above change, I think as a consequence quite some parts of the code & tests need to be changed & updated. Do you have an idea on how we could split the introduction of them into separate PRs so we don't have one big PR?

[sunshinejr]

The cases are:

1. I was thinking of producing a default parameter encoding to make it easier to use (most of the time you use one specific encoding), maybe it can be customizable in MoyaProvider like provider.defaultParameterEncoding, or maybe we should just use URLEncoding.default like it is in endpointClosure by default right now.
2. In the other two cases you can also specify parameters in your download requests. Download request is almost the same as normal one that doesn't save anything to disk, so we might also add these options to them if needed.
3. Yeah, this is the trickiest one - we probably need this parameter encoding there too.

So about PRs - I'm thinking about one big PR that we can target in the smaller PRs. This is because we probably can't do few PRs without breaking a build for some time.

Also, I would love to hear feedback from more @Moya/contributors since it is kinda big change, and if we agree on something, we may want to implement it in 9.0.0 as well.

[Jeehut]

Regarding Point No. 1

I understand what you mean. I think expecting there to be a defaultParameterEncoding present for this case will be hard for the user to understand/memorize.

Here's my alternative suggestion:

Let's add a defaultParameterEncoding to the TargetFile and provide a default implementation for it that simply returns URLEncoding.default.

Additionally let's entirely remove the parameters([String: Any] case. Instead users would be able to use the case encoded(parameters: [String: Any], encoding: ParameterEncoding) and provide defaultParameterEncoding for the encoding: parameter of that case. This would prevent any confusion and even make the existence/usage of the encoding explicit.

[Jeehut]

Regarding Point No. 2: I understand now and this totally makes sense (sounds like it was quite annoying to specify params alongside a download request?).

Regarding Point No 3: Okay, let's add the bodyEncoding there then. If we use my defaultParameterEncoding suggested above, this would be useful here, too.

[Jeehut]

Please note that there is now https://github.com/Alamofire/Alamofire/issues/2181 around which tracks support for the Encodable type right within Alamofire. As it is our main dependency, I think their approach to integrate this feature should be considered before finally deciding on how to implement this.

Please also note that the contributors have already confirmed that they have plans to support both Encodable and Decodable types in release version 5 in https://github.com/Alamofire/Alamofire/issues/2177.

[Jeehut]

I've just created #1147 which implements @sunshinejr's suggestions with my suggested changes above. Note that I decided to make two more changes than explained above:

1. Rename RequestType to RequestDataType since there was a name collision with a type in the Result framework
2. I've split composite up into compositeEncoded and compositeData since that is more consistent in my opinion (someone might need url parameters using Data as well)

Here's what the above four types now look like:

/// Represents a type of upload task.
public enum UploadType {

    /// Upload a file.
    case file(URL)

    /// Upload "multipart/form-data"
    case multipart([MultipartFormData])
}

/// Represents a type of download task.
public enum DownloadType {

    /// Download a file to a destination.
    case destination(DownloadDestination)

    /// Download a file to a destination with extra parameters using the given encoding.
    case encoded(DownloadDestination, parameters: [String: Any], encoding: ParameterEncoding)
}


/// Represents an HTTP task.
public enum Task {

    /// A basic request task.
    case request(RequestDataType)

    /// An upload task.
    case upload(UploadType)

    /// A download task.
    case download(DownloadType)
}

/// Represents a type of request.
public enum RequestDataType {

    /// A requests body set with data.
    case data(Data)

    /// A requests body set with parameters and encoding.
    case encoded(parameters: [String: Any], encoding: ParameterEncoding)

    /// A requests body set with data, combined with url parameters.
    case compositeData(urlParameters: [String: Any], bodyData: Data)

    /// A requests body set with parameters and encoding, combined with url parameters.
    case compositeEncoded(urlParameters: [String: Any], bodyParameters: [String: Any], bodyEncoding: ParameterEncoding)
}

[Jeehut]

This was fixed via #1147.