kean / Get

Web API client built using async/await
MIT License
937 stars 74 forks source link

Improve Request type #51

Closed kean closed 2 years ago

kean commented 2 years ago

This MR addresses multiple design issues with the Request type in Get 1.0.

1. Add support for both URL and String

In Get 1.0, request can only be initialized with a String that represents either a relative URL (path) or an absolute one.

let request = Request(url: "/user")
let request = Request(url: "https://api.github.com/user")

This doesn't match the convention used on Apple platforms where URLs are usually represented using URL type. Get 2.0 now has two separate initializer:

let request = Request(path: "/user")
let request = Request(url: URL(string: "https://api.github.com/user"))

// Relative URLs can also be represented using `URL`:
let request = Request(url: URL(string: "/user"))

2. Deprecate telescoping factory methods

// Deprecated in Get 1.0
@available(*, deprecated, message: "Please use Request initializer instead")
extension Request {
    public static func get(_ path: String, query: [(String, String?)]? = nil, headers: [String: String]? = nil) -> Request {
        Request(path: path, method: .get, query: query, headers: headers)
    }

    public static func post(_ path: String, query: [(String, String?)]? = nil, body: Encodable? = nil, headers: [String: String]? = nil) -> Request {
        Request(path: path, method: .post, query: query, body: body, headers: headers)
    }

    ...
}

Having a separate factory method per HTTP method is not elegant. Initially it was designed this way to prevent "GET" requests from being sent with an HTTP body which is not allowed by the spec and by URLSession. But since the Request type also has a public initializer without this check, it doesn't make much sense to only have it in one place.

The factory methods are deprecated in Get 2.0 and there is also a new HTTPMethod type.

let request = Request(path: "/user")
let request = Request(path: "/user", method: .post)

This is more verbose than writing .post("/user"), but I think it's a non-issue, and the clarify should be the priority.

3. Change the order of parameters

Previously, method was the first parameter, but it had a default value. The first parameter should not have the default value.

// Before
let request = Request(method: "GET", url: "/user")

// After
let request = Request(path: "/user", method: .post)

4. Response type defaulting to Void

In Get 1.0, there were actually not one, but two sets of factory methods (see p2). In the second set, the default response type (Request<Response>) was set to Void. It allows the user to avoid explicitly specifying the response type when it's Void. But the initializer for the same type was missing, and Get 2.0 introduces it, so you can now write this:

// Previously was a compiler error
let request = Request(path: "/user", method: .post)

Rejected

The Request initializer has a lot of parameters and it can be cumbersome to use. I considered adding some convenience methods for creating the requests, for example:

let request = Request(path: "/user")
    .query([("key", "value")])
    .body("hello")

While it's nice to have, I don't think it belongs in the framework. You can achieve the same with simple mutable structs in Swift and the users can add convenience extensions "aftermarket":

var request = Request(path: "/user")
request.query = [("key", "value")]
request.body = "hello"
LePips commented 2 years ago

Just a few questions but 👍 for everything moving forward!

prevent "GET" requests from being sent with an HTTP body which is not allowed by the spec

My understanding of RFC 7231 is that GET requests can have a body but should be ignored.

kean commented 2 years ago

My understanding of RFC 7231 is that GET requests can have a body but should be ignored.

URLSession immediately fails the requests with a body.

2022-08-18 07:49:19.795866-0400 xctest[99043:11389881] GET method must not have a body 2022-08-18 07:49:19.797580-0400 xctest[99043:11389879] Task <15BA91AD-79A2-48C4-BCC2-29F19BDE12D2>.<1> finished with error [-1103] Error Domain=NSURLErrorDomain Code=-1103 "resource exceeds maximum size" UserInfo={NSLocalizedDescription=resource exceeds maximum size, NSErrorFailingURLStringKey=https://api.github.com/user, NSErrorFailingURLKey=https://api.github.com/user, _NSURLErrorRelatedURLSessionTaskErrorKey=( "LocalDataTask <15BA91AD-79A2-48C4-BCC2-29F19BDE12D2>.<1>" ), _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <15BA91AD-79A2-48C4-BCC2-29F19BDE12D2>.<1>, NSUnderlyingError=0x600000c60570 {Error Domain=kCFErrorDomainCFNetwork Code=-1103 "(null)"}}

I'm not sure what's the reasoning. I'm assuming it's caching. URLs must uniquely identify the resources.