swift-server/async-http-client

HTTPClientRequest looks & feels like a value type but isn't

weissi opened this issue · 10 comments

weissi commented

Sadly HTTPClientRequest contains a streamable body which is typically a reference to a (frequently) consume-once AsyncSequence. That means it's not actually a value type.

That's a major API winkle that should be fixed as soon as feasible.

My recommendation would be to separate the request and the body. Something like try await httpClient.execute(request, body: myStream).

dnadoba commented

Agree, same is true for the response. We should do it at the same time we adopt swift-http-types.

  • ❤️1
weissi commented

Indeed, swift-http-types adoption is the right cause of action here. Maybe we can even do this without a major then. Can we just deprecate HTTPClientRequest/Reponse and migrate to http types?

dnadoba commented

Currently yes but not after your PR has landed.

weissi commented

Currently yes but not after your PR has landed.

Oh really, what changes?

dnadoba commented

Currently HTTPClientRequest can be split into <Body: AsyncSequence>(HTTPRequest, Body) but afterwards it would be <Body: AsyncSequence>(HTTPRequest, Body, TLSConfiguration?).

weissi commented

Currently HTTPClientRequest can be split into <Body: AsyncSequence>(HTTPRequest, Body) but afterwards it would be <Body: AsyncSequence>(HTTPRequest, Body, TLSConfiguration?).

Cool, that seems to work either way.

dnadoba commented

It doesn't scale well. Why is TLSConfiguration now an argument but the rest of the HTTPClient.Configuration not?
I think your proposed solution in #392 is the way forward with the exception that tier 3 is just (HTTPRequest, Body) which is given to tier 2 to execute.
If you want to change the TLSConfiguration you just mutate a local copy of your tier 2 value.

weissi commented

It doesn't scale well. Why is TLSConfiguration now an argument but the rest of the HTTPClient.Configuration not? I think the your proposed solution in #392 is the way forward with the exception that tier 3 is just (HTTPRequest, Body) which is given to tier 2 to execute. If you want to change the TLSConfiguration you just mutate a local copy of your tier 2 value.

Right, there's still an API-design challenge but #709 doesn't change anything about it because it amends the to-be-deprecated HTTPClientRequest.

fumoboy007 commented

Agree, same is true for the response. We should do it at the same time we adopt swift-http-types.

Hi @dnadoba! Is this the issue that I should subscribe to for swift-http-types adoption or is there a separate issue?

dnadoba commented

That’s the right issue to track :)

  • ❤️1