upgrade hyper to v0.11 #151
Conversation
|
Woops, forgot I had disabled the |
There was a problem hiding this comment.
I've just done a quick poke through the code and left some comments.
They're probably mostly just me being ignorant :) I did expect the synchronous layer to be a bit thinner, and confused myself with the async methods peppered around. Is the idea that the asynchronous Body is an implementation detail and you always work with the synchronous one?
Maybe adding a simple asynchronous post example would make that all clearer.
src/async_impl/body.rs
Outdated
| } | ||
| } | ||
|
|
||
| // pub(crate) |
There was a problem hiding this comment.
pub(crate) is stable now, right?
There was a problem hiding this comment.
On most recent version. It doesn't really cost anything to still compile on the previous version.
| // pub(crate) | ||
|
|
||
| #[inline] | ||
| pub fn wrap(body: ::hyper::Body) -> Body { |
There was a problem hiding this comment.
Any reason not to make these inherent methods?
There was a problem hiding this comment.
Yep, because they're pub(crate). Implementation details.
There was a problem hiding this comment.
Ohhh. I actually totally misunderstood the // pub(crate) comment. I thought it was a reminder to make these pub(crate) once that feature was stable, not to say the following pub items are not part of the public API
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn can_reset(body: &Body) -> bool { | ||
| #[inline] | ||
| pub fn async(body: Body) -> (Option<Sender>, async_impl::Body, Option<u64>) { |
There was a problem hiding this comment.
Should this be public, since it's returning an unstable async Body?
There was a problem hiding this comment.
The body module isn't public, so these functions are pub(crate).
| /// Relies on the `futures` crate, which is unstable, hence this module | ||
| /// is unstable. | ||
| pub mod async { | ||
| pub use ::async_impl::{ |
There was a problem hiding this comment.
Should this also include RequestBuilder and Body?
src/async_impl/body.rs
Outdated
| use futures::{Stream, Poll, Async}; | ||
| use bytes::Bytes; | ||
|
|
||
| pub struct Body { |
There was a problem hiding this comment.
It seems like a shame to lose some of the From conversions on the synchronous body when dealing with the asynchronous one directly.
There was a problem hiding this comment.
Indeed, but asynchronous bodies need to be Stream, and that means File and Box<Read> and friends don't fit, yet. I wanted to get the upgrade done before figuring out all that.
|
nice, I'm waiting for this :D! |
This adds a
reqwest::unstable::asyncmodule that makes puts the reqwest API over hyper v0.11, but with futures and streams. The sync API is powered by the unstable async one. This should keep the sync API completely the same, with all features working.The async module is available publicly only with
features = ["unstable"]in yourCargo.toml, and then viareqwest::unstable::async. It may be worth considering how rayon handles unstable APIs, and possibly do likewise.The async module may be missing some features. The gzip option does not work, as the libflate crate used internally cannot handle non-blocking IO yet. The
Bodystream may be barebones, while trying to decide the exact async body API.