http-rs / surf

Fast and friendly HTTP client framework for async Rust
https://docs.rs/surf
Apache License 2.0
1.46k stars 120 forks source link

Rework `Request` (and related) #192

Closed Fishrock123 closed 4 years ago

Fishrock123 commented 4 years ago

This is essentially a summary of the major improvements to Surf I've been trying to make. there is a lot of overlap with disparate existing issues, which I've linked where appropriate.


In order to improve middleware for Surf (https://github.com/http-rs/surf/issues/131), and bring it more in-line with Tide, the following changes should be made:

And also for code hygiene, Request should probably not be all of these at once (https://github.com/http-rs/surf/issues/152):

Additionally related, Client should not be generic (https://github.com/http-rs/surf/issues/69), (comment on 105), and Middleware may want to be accessible from the present Middleware stack for complete retry/redirect purposes (https://github.com/http-rs/surf/issues/169), (https://github.com/http-rs/surf/issues/18).

Note: Making Client 1:1 with an actualized Request helps this but doesn't work for keep-alive or http2/3+.


There is a clean but slightly less ergonomic way to solve most of the API issues here:

let res = surf::post(url)
    .header(h, v)
    .body(b)
    .send().await?;

With middleware:

let req = surf::post(url)
    .header(h, v)
    .body(b);
let client = surf::client()
    .middleware(m);
let res = client.send(req).await?;

Regarding Client being generic, I see two paths. Either #69, or just making the structure's properties be directly determined by compile-time configuration. dyn is probably better if we want people to be able to pass external clients that implement some kind of trait. In that case, surf::client_from(trait) or similar could exist, or possibly even .send::<ClientType>(). Update: see https://github.com/http-rs/surf/pull/193 "dyn HttpClient". (Merged)

Ideally I think that Client would be the thing that actualizes/sends Requests and holds the middleware stack, similar to what https://github.com/http-rs/surf/issues/109 asks for.


This was an older WIP, see #194 for a full compiling patch of the above proposal! I have a WIP patch of some of this stuff, but in the form of a `SurfBuilder`, which has a structure like so and is not much cleaner than today, although it does allow middleware to operate on `surf::Request` and `surf::Response`. **I prefer the approach I described above.** ```rust // not great pub struct SurfBuilder { client: C, req: Option, middleware: Option>>>, fut: Option>>, } ```

tl;dr things become much cleaner and simpler if:

goto-bus-stop commented 4 years ago

great post, i think i agree with this direction. thanks for writing this up!

should we still have surf::post at all if it's just a shorthand for like, surf::RequestBuilder::post()? :)

Have Client::send(req: impl Into<surf::Request>) -> BoxedFuture rather than making anything be directly await-able.

strongly agree with this in particular—it makes the API a bit less "nice" (for some values of nice) but it's better both for the implementation and for actually modeling how requests are sent (client sends requests, requests don't send themselves).

Fishrock123 commented 4 years ago

Done in #194.