Improve Request type

[kean]

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]

Is there a motive for optional? I would implicitly expect that the developer would make sure that the request is valid to begin with. We could mimic the final URL construction on Requests.swift L443 and throw URLError(.badURL) with init(path: String ...), but I don't think I like one init throwable and the other not.

This would possibly allow some cleanup in makeURL regarding your TODO.

[kean]

The initializer that accepts path as a String can fail to produce a URL:

public init(path: String, method: HTTPMethod = .get, ...) {
    self.url = URL(string: path) // Optional

[LePips]

Think that these deprecated methods can be removed in 2.0?

[kean]

Yes, absolutely!

[LePips]

What if this was [URLQueryItem]? on Request but with convenience inits? I ask only because I personally don't work much with tuples.

[kean]

I also considered it, and I'm not sure yet. Tuples are probably not the ideal choice for this case. When I think about tuples, I usually think about a bag of values that are loosely related and there is no good name for them. That's not the case for the query items.

var request = Request(path: "/domain.tld")
request.query = [("name", "value1+value2")]
request.query = [URLQueryItem(name: "name", value: "value1+value2")]

[LePips]

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.

[liamnichols]

LGTM 👍 a ExpressibleByStringLiteral HTTPMethod struct is how our pre-existing networking client was build while remaining compatible with CreateAPI so I like that approach being taken here

[liamnichols]

This makes sense to me. I was a bit confused that path had been renamed to url in 1.0, but it makes sense given the dual purpose.. But changing the underlying type to URL and having this initialiser sounds good in terms of CreateAPI usage 👍

Just to confirm though, this is targeting version 2.0.0?

[kean]

Yes, it used to serve both purposes, but both poorly. The new API should be much clearer.

Yes, I know I just released 1.0, but I want to address these issues earlier. Yes, I'll release it in a 2.0 because there are some breaking changes.

[LePips]

Just my opinion, since there are already breaking changes I think it would be okay to just remove the old telescoping methods instead of at a later date. This is a foundational change and developers would have to be responsible for making the 2.0 major update.

[kean]

I agree, I don't want to pollute the APIs, and it's still at the early stage.

[kean]

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.