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

Split sync and async response APIs #202

Closed
sagebind opened this issue Jun 28, 2020 · 0 comments · Fixed by #262
Closed

Split sync and async response APIs #202

sagebind opened this issue Jun 28, 2020 · 0 comments · Fixed by #262
Labels
breaking Requires breaking backwards compatibility enhancement Make a feature better
Milestone

Comments

@sagebind
Copy link
Owner

While it is very convenient for Isahc to support fully both sync and async APIs under the same traits, it also can cause some confusion for users instead of steering them into the pit of success. In particular, ResponseExt exposes synchronous APIs that are very easy for users to accidentally use inside an async context, when they actually intended or ought to use an asynchronous equivalent.

We should split our Body type into a sync an async variant so that impl Read and impl AsyncRead is not both true for one type. To permit the appropriate response methods, send will return a Response<Body> while send_async will return a Future<Item = Response<AsyncBody>>. This will help users use the sync or async methods correlating with how the request was sent originally.

If users need to switch between sync and async APIs intentionally, we can provide Body::into_async and AsyncBody::into_sync or something like that to switch between the two, since the underlying client doesn't really care how the response is consumed and you can easily switch between the two.

We should also perhaps move all the async methods on ResponseExt into a new trait AsyncResponseExt, which would allow us to drop the _async suffix for the async versions.

@sagebind sagebind added enhancement Make a feature better breaking Requires breaking backwards compatibility labels Jun 28, 2020
@sagebind sagebind added this to the 0.10 milestone Jun 28, 2020
@sagebind sagebind modified the milestones: 0.10, 1.0 Jul 29, 2020
@sagebind sagebind mentioned this issue Jul 29, 2020
8 tasks
sagebind added a commit that referenced this issue Nov 25, 2020
sagebind added a commit that referenced this issue Nov 28, 2020
Make the big split between synchronous responses and asynchronous responses, which offers ergonomic improvements and encourages users towards using the right methods by default as discussed in #202. This required the following changes:

- Split `Body` into two distinct types: `AsyncBody` and `Body`, which are asynchronous only and synchronous only, respectively.
- Split `ResponseExt` into three traits: `ResponseExt` for methods that don't use the body, `ReadResponseExt` for synchronous methods that read from the body, and `AsyncReadResponseExt` for async methods that read from the body. This offers some ergonomic improvements as well, as the async response methods can drop the `_async` suffix, and Rust will automatically resolve to the correct method depending on whether the response is async or not.

This split also enables a new capability that I did not think of before, which is that you can now use anything implementing `Read` as a request body such as `File` when using the synchronous API. For those not using async at all, this is a significant improvement!

Fixes #202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Requires breaking backwards compatibility enhancement Make a feature better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant