API semantics of request method in URLRoutingClient

[Jeehut]

I just watched your latest episode and while I loved the whole idea of keeping your API client in sync with the server & how mocking is solved in _URLRouting, I do have an issue with the API semantics of the request(_:as:decoder:) method, see this screenshot:

To me this feels wrong for the following reasons:

1. It's not clear if calling request(...) returns a request object similar to URLRequest or executes a request and returns some kind of response. This is a common issue with English words that can be a verb or a noun, as some APIs prefer describing the return type and others prefer describing the side effects (which is what you do here).
2. For async APIs, Apple actually prefers describing the return type, see for example the URLSession.data(for:delegate) API or the fresh MusicCatalogSearchRequest.response() API from MusicKit introduced during WWDC2021. So Apple platform developers might expect that this to return some kind of Request object, like NSManagedObject.fetchRequest().
3. The API is throwing which is fine for those who prefer throwing APIs, but in my own Twitter survey I learned that (at least in my bubble) more developers prefer Result APIs because we don't have precise error typing in Swift yet unless we return Result.failure instead of throwing. The cool thing about returning Result instead of throwing is also, that users get a throwing API for free because they can just call get() on the Result.
4. The as naming makes the function read request as which is not that bad, but still lacks some clarity in my opinion. It could be easily mixed up with the formatting of the body, for example, so some might try writing request(..., as: .json). In my own networking library I named that parameter decodeBodyTo which could fit here, too.
5. The decoder currently needs to be provided with every single request, but it is very common to reuse one decoder for all request of a specific server. So much so, that I think the library should provide a defaultDecoder property on the URLRoutingClient level in my opinion. This could still be overridden when needed on a per-call basis.

So putting all these changes together, I suggest to change the semantics of the request method from:

  public func request<Value: Decodable>(
    _ route: Route,
    as type: Value.Type = Value.self,
    decoder: JSONDecoder = .init()
  ) async throws -> (value: Value, response: URLResponse) {

To this:

  public func response<Value: Decodable>(
    _ route: Route,
    decodeBodyTo type: Value.Type = Value.self,
    customDecoder: JSONDecoder? = nil
  ) async -> Result<(value: Value, response: URLResponse), URLRoutingError > {

Where the error type could look something like this:

public enum URLRoutingError {
  case decodingError(URLRoutingDecodingError)
  case urlError(URLError)
  case transportError(Error)
  case serverSideError(Int)
}

The error type is just a quick and dirty suggestion, we could provide a more sophisticated error type covering all common error reasons to provide users with a more helpful explicit error type, something like my own ApiError type but adjusted to this library.

Would love to hear your thoughts!

[stephencelis]

Hi @Jeehut! Thanks for the discussion and feedback! I could definitely see some tweaking here. Maybe something like:

let loginResponse = try await client.response(for: .login, as: LoginResponse.self).value

To respond more directly to a few of your items:

3. The API is throwing which is fine for those who prefer throwing APIs, but in my own Twitter survey I learned that (at least in my bubble) more developers prefer Result APIs because we don't have precise error typing in Swift yet unless we return Result.failure instead of throwing. The cool thing about returning Result instead of throwing is also, that users get a throwing API for free because they can just call get() on the Result.

I definitely agree that typed errors can be handy, but we're trying to follow Apple's lead here, which is to prefer throwing functions. Luckily I think you can extend URLRoutingClient yourself with a non-throwing version that does the same work.

4. The as naming makes the function read request as which is not that bad, but still lacks some clarity in my opinion. It could be easily mixed up with the formatting of the body, for example, so some might try writing request(..., as: .json). In my own networking library I named that parameter decodeBodyTo which could fit here, too.

We looked to prior art here. In particular, I think pointer.load(as:) and maybe String.init(decoding:as:) (which is admittedly a little different). decodeBodyTo is descriptive but doesn't quite feel like something that'd appear in the standard library. Any other ideas?

5. The decoder currently needs to be provided with every single request, but it is very common to reuse one decoder for all request of a specific server. So much so, that I think the library should provide a defaultDecoder property on the URLRoutingClient level in my opinion. This could still be overridden when needed on a per-call basis.

The client-level JSON decoder was actually added in this PR:

#6

We just need to cut a new version for it.

As for decoder as the name at this level, we're still going to look to prior art here, for example in Combine's decode method on publishers, and we're not sure more needs to be qualified. Have you come across examples of customDecoder?

[Jeehut]

The actual part that caused me to start this discussion was the request part (as you can see highlighted in the screenshot), the rest just came to me as additional potential improvements while writing the text. You're right, I can wrap things to get a Result returning function myself (and I will do), I just thought you might want to provide it yourself.

And Apple isn't the only source for inspiration to me, they often times don't adhere to their own advices. I only try to make sure not to interfere with expectations of developers who are used to Apples APIs, I often opt for more descriptive names and more type safety then they suggest in their APIs/examples. Which by the way is why I love your frameworks because they do, too.

The customDecoder was just one idea to communicate that this doesn't need to be set in most cases. I personally will probably never need it at all, so I'd be happy just having a decoder on the URLRouting level. but that's just me. Alternative names I considered suggesting were overrideDecoder and forceDecoder. I don't have any hard feelings here though.

Same for the as naming, I don't have any strong feelings there, I just prefer more descriptive names.

Overall I like the code example above much better than the current API. I just noticed though that the tuple returns a response which would lead to a weird duplicate call like client.response(...).response. A solution could be to rename the response in the tuple to something like httpUrlResponse, fullResponse or responseObject. Or the function just gets renamed to be more explicit about the side effect, e.g. something like performRequest.

While looking at the tuple, I also just thought value could be named to something more descriptive like decodedValue, decodedBody or typedValue. But yeah, I guess we shouldn't over-think it and keep it simple.