Feedback, some ideas for improvements

[kevinrenskers]

So far I am really liking this library, well done!

However, there is one thing that is kind of bugging me: making the request structs gets tedious, and takes quite a lot of lines of code.

struct LoginTokenRequest: JsonRequest {
  private struct Payload: Encodable {
    let email: String
    let password: String
  }

  typealias Result = Token

  let email: String
  let password: String

  var method: HttpMethod = .post
  var routes: HttpRoute = ["auth", "tokens"]

  var body: HttpBody {
    HttpData.Json(Payload(email: email, password: password))
  }
}

Now imagine having tens of requests - this gets so long so fast.

A few observations.

It's a bit annoying that the HttpData.Json payload needs to be an Encodable thing. This causes the need for one-off payload structs, and repeated code to copy the properties over. On the other hand, HttpQuery can be created using a simple dictionary. Is it an idea to add an initializer that takes a dictionary instead of an Encodable struct/object? That way we could get rid of the Payload struct in my example. Turning a dict into Data is simple enough of course.

Or... could LoginTokenRequest itself be the payload? It has the email and password properties after all, so in theory it could be the payload? 🤔 Like, any properties on the request will be sent as body payload. Not sure if that's a terrible idea haha.

Another thing that could work to reduce the code is if JsonRequest wasn't a protocol but a Struct - that way you could simply create them using an initializer, have them in an enum, whatever.

I tried to implement something like that myself, but the typealias Result is making me hit roadblocks:

struct CoreRequest<Result: Decodable>: JsonRequest {
  var method: HttpMethod
  var routes: HttpRoute
  var body: HttpBody
}

enum Requests {
  case getLoginToken(email: String, password: String)

  var request: CoreRequest {
    switch self {
    case .getLoginToken(let email, let password):
      return CoreRequest<Token>(method: .post, routes: ["auth", "tokens"], body: HttpData.Json(payload))
    }
  }
}

As you can see, having 10 or 20 requests like this vs the "normal" method of one struct per request would save a LOT of repeating typing. But sadly this doesn't work since there's the generic type requirement. I would love to get this to work though. See also https://github.com/gonzalezreal/SimpleNetworking for inspiration.

Very curious about your thoughts and I'd be happy to brainstorm some ideas, test any of them, etc.

[kevinrenskers]

Okay, this works:

struct CoreRequest<Result: Decodable>: JsonRequest {
  var method: HttpMethod
  var routes: HttpRoute
  var body: HttpBody
}

private struct LoginTokenPayload: Encodable {
  let email: String
  let password: String
}

extension CoreRequest {
  static func getLoginToken(email: String, password: String) -> CoreRequest<Token> {
    return CoreRequest<Token>(method: .post, routes: ["auth", "tokens"], body: HttpData.Json(LoginTokenPayload(email: email, password: password)))
  }
}

But that LoginTokenPayload is still quite annoying and causes so much repeated code of setting email and password over and over.

But the main thing it solves is that creating another request is now a lot simpler: less lines, and auto-completion works a lot nicer too.

[kevinrenskers]

And also created this extension to create payload directly using a dictionary:

extension HttpData {
  public struct Dictionary: HttpBody {
    private let value: [String: Any]

    public init(_ value: [String: Any]) {
      self.value = value
    }

    public func add(to request: inout URLRequest) throws {
      request.addValue(
        HttpMimeType.json.rawValue, forHTTPHeaderField: "Content-Type"
      )
      let jsonData = try JSONSerialization.data(withJSONObject: value, options: .prettyPrinted)
      request.httpBody = jsonData
    }
  }
}

I think with that I solved all the major problems I had, without needing a single library change :)

[borchero]

Happy to get so much feedback from you ;) some comments:

• HttpData.Json requires an Encodable, however, Dictionary is also Encodable, so you can just pass a dictionary to its initializer. Similarly, nothing prevents you from letting a Request itself conform to Encodable and then pass self to the initializer of HttpData.Json. Depending on your preference, you can thus easily get rid of your Payload struct.
• As a result, I don't really see the need for something like HttpData.Dictionary. In this instance it would also not be obvious which protocol to use for encoding (i.e. whether we would use JSON or something else).
• There is something very similar to your CoreRequest in Squid: AnyRequest lets you create requests directly. If you go for your Requests enum, you could just replace the return value CoreRequest by some Request (I'm not sure if you're familiar with the some keyword - essentially, it lets you return a protocol with associated types, see https://github.com/apple/swift-evolution/blob/master/proposals/0244-opaque-result-types.md). Nonetheless, I would prefer the static method on some singleton class.

As a quick summary of my comments, you make take a look at the following code:

enum Requests {

    case getLoginToken(email: String, password: String)

    var request: some Request {
        switch self {
        case .getLoginToken(let email, let password):
            return AnyRequest(
                .post,
                url: "myapi.example.com/auth/tokens",
                body: HttpData.Json(["email": email, "password": password])
            )
        }
    }
}

[kevinrenskers]

Thanks for the comments! I missed AnyRequest while going through the docs, but now that I found it, I don't think I'll use it.

This request fixes the result type to Data such that the user can decode to the desired type via calling the decode(type:decoder:) function on the returned Response publisher when scheduling the request.

Also, having to give it a full url instead of a route is not so great for my use case. I think my CoreRequest does solve my problems just a little bit better - for my use case.

however, Dictionary is also Encodable

🤯 Okay, I didn't know that one yet haha

And yeah I am also preferring static methods over an enum. Enum just causes duplication for no real win (unless you want to go the Moya route where you have to give params, headers, urls etc etc separately).

[kevinrenskers]

Hm sadly I am still running into issues. I am guessing this is why you have AnyRequest have a fixed result type.

let request = CoreRequest.getLoginToken(email: email, password: password)
// error: Generic parameter 'Result' could not be inferred

Which seems weird to me, since it's right there?

static func getLoginToken(email: String, password: String) -> CoreRequest<Token> {
  return CoreRequest<Token>(method: .post, routes: ["auth", "tokens"], body: HttpData.Json(["email": email, "password": password]))
}

I know this isn't a bug with your library or even a feature request, so feel free to just close and shut down this GH issue :)

[borchero]

The last bug you described is a "problem" with Swift - since you're calling a static function on a generic type (aka CoreRequest), you have to give the generic parameter when calling the static function, i.e.:

let request = CoreRequest<Token>.whatever()

Obviously, this defeats the purpose, so in your case, I would suggest to just create some "singleton" class (maybe RequestFactory?) and define static methods returning CoreRequest instances on that type.