Moya: Add Moya.Response extension for Decodable types in Swift 4

[SD10]

It seems like handling response JSON is a highly debated topic due to there being so many JSON serialization libraries and no determined best practice.

However, with the addition of the Codable protocol to the Swift 4 Standard Library, I think this will become a widely used approach. I'm wondering if the Moya community would be willing to add some basic extensions to Moya.Response to map Decodable types.

Since Codable is part of the Standard Library I think it's a safe addition to Moya's core.

A rough API would be:

    func mapObject<D: Decodable>(with decoder: JSONDecoder) throws -> D {}
    func mapObject<D: Decodable>(atKeyPath keyPath: String, with decoder: JSONDecoder) throws -> D {}

Moya already supports mapping to JSON, Images, and Strings

Regardless, these are not difficult extensions to write on our own or provide in a community extension.

[BasThomas]

Hmm, this is an interesting one. Not sure what to think of it yet.

[sunshinejr]

Thanks for the issue, @SD10 - this is a really good idea. Having standardized encoding/decoding by Swift standard library, I think it is natural for us to implement a basic map method that would cover this behavior. However, for now we don't have any Swift 4.0 branch and we are in the middle of Moya 9.0 release, so we might want to wait with that implementation.

[SD10]

@sunshinejr thank you for the feedback. I would be willing to provide the implementation for this feature when Moya is ready to start the Swift 4 migration. Including writing the Rx/Reactive extensions, documentation, and testing. If there's anything else I can do the contribute in the meantime, please feel free to let me know.

[AndrewSB]

Thanks for taking the initiative @SD10! That sounds great, we'll update this issue once we cut a 9.0 release 😄

[Jeehut]

Please note that there is now https://github.com/Alamofire/Alamofire/issues/2180 around which tracks support for the Decodable 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.

[SD10]

@devxoul You may be interested in working on this one? I just saw RxCodable.

I have a branch that's part way through on my local copy but I thought I'd get your thoughts. Not trying to dictate contributions, just thought you may be interested 😃

[devxoul]

@SD10 It looks interesting. I think I can do it :)

[sunshinejr]

Hey @devxoul, just making sure - do you think you will want to implement this one? If not we might want to add labels to seek for help 😉

[devxoul]

@sunshinejr, oh yeah I'm currently working on it. I'll make a pull request in this week :)

[sunshinejr]

That's great! I assigned you to this task then 😉

[devxoul]

1335 :tada:

[SD10]

Closing this in favor of #1335. Let's move the discussion there 🎉

[BasThomas]

Why do we close these issues if they get auto-closed after the PR is merged? Now, if for some reason the PR isn't merged, there's a big chance this issue gets overlooked and stays closed.

[SD10]

Good point @BasThomas. I always thought it was to encourage discussion on the PR and not have it split across two places. Feel free to reopen this and maybe we can default to the policy you described

_{Sent with GitHawk}

[BasThomas]

I can't remember this happening somewhere actually, and I feel like the auto-closing works best. But if I am in a minority here, I'd not want to change the way this is handled now. @Moya/contributors, what do you think?

[devxoul]

I think we should keep this issue open because...

1. there could be another pull request in different idea.
2. this issue is not yet completely finished.

Also this is will be automatically closed if #1335 is merged.

[BasThomas]

We can close this now :)

[devxoul]

Hmm I have no idea why GitHub didn't close this issue automatically when #1335 is merged 🤔

[BasThomas]

Think it was confused because we closed an reopened manually 😛

[devxoul]

Ah, that makes sense!