Feedback, some ideas for improvements
kevinrenskers opened this issue · 6 comments
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.
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.
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 :)
Happy to get so much feedback from you ;) some comments:
HttpData.Json
requires anEncodable
, however,Dictionary
is alsoEncodable
, so you can just pass a dictionary to its initializer. Similarly, nothing prevents you from letting aRequest
itself conform toEncodable
and then passself
to the initializer ofHttpData.Json
. Depending on your preference, you can thus easily get rid of yourPayload
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 yourRequests
enum, you could just replace the return valueCoreRequest
bysome Request
(I'm not sure if you're familiar with thesome
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])
)
}
}
}
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).
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 :)
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.