Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
weissi opened this issue Aug 15, 2023 · 10 comments
Open

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

weissi opened this issue Aug 15, 2023 · 10 comments
Labels
bug Something isn't working

Comments

@weissi
Copy link
Contributor

weissi commented Aug 15, 2023

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).

@weissi weissi added the bug Something isn't working label Aug 15, 2023
@dnadoba
Copy link
Collaborator

dnadoba commented Aug 15, 2023

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

@weissi
Copy link
Contributor Author

weissi commented Aug 15, 2023

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
Copy link
Collaborator

dnadoba commented Aug 15, 2023

Currently yes but not after your PR has landed.

@weissi
Copy link
Contributor Author

weissi commented Aug 15, 2023

Currently yes but not after your PR has landed.

Oh really, what changes?

@dnadoba
Copy link
Collaborator

dnadoba commented Aug 15, 2023

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

@weissi
Copy link
Contributor Author

weissi commented Aug 15, 2023

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
Copy link
Collaborator

dnadoba commented Aug 15, 2023

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
Copy link
Contributor Author

weissi commented Aug 15, 2023

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
Copy link

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
Copy link
Collaborator

dnadoba commented Dec 28, 2023

That’s the right issue to track :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants