http-rs / surf

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

src: overhaul Client, rework Request and Middleware, add builder #194

Closed Fishrock123 closed 4 years ago

Fishrock123 commented 4 years ago

A practical implementation of https://github.com/http-rs/surf/issues/192

Please see that issue for full background on this PR.

This includes the following changes:

Note: The client: Client passed to Middleware uses the same underlying HttpClient but does not preserve the Middleware stack. I do not know if it is desirable to keep it, and much less if it is possible to make such a thing work with the borrow checker.

This is absolutely semver-major.

Edit to be clear: The API for one-offs looks almost identical:

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

Re-use of a Client or use of Middleware now looks like this:

let client = surf::client()
    .middleware(m);

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

Note: This presently causes stack overflows on the tests. I'm not sure why but I suspect it has to do with the middleware changes. It was an accidental recursive Client::send.

Fishrock123 commented 4 years ago

I have made a possible additional patch at https://github.com/Fishrock123/surf/pull/1 that re-adds surf::{method}().middleware() in the form of RequestBuilder::middleware(), and an Option<Vec<Arc<dyn Middleware>>> on RequestBuilder.

Note it poses the following question if applied:

  • What to do if Request is used in an existing Client? The middleware would be lost. Maybe panic. Maybe do nothing.
Fishrock123 commented 4 years ago

I would like to make it clear that some form of this is necessary in order to have middleware use surf::Request and surf::Response rather than the http_types structs (https://github.com/http-rs/surf/issues/131).

Maybe the "builder" can be extended more to have more existing api parity or whatever, but the Request struct itself still need to change.

Fishrock123 commented 4 years ago

@yoshuawuyts please take a look at this

Fishrock123 commented 4 years ago

I've made a second additional patch to https://github.com/Fishrock123/surf/pull/1 which re-adds support for per-request middleware. I don't fancy the patch and think it is probably not what we should encourage users to do, ergo a mistake, but if people like it, then... 🤷

goto-bus-stop commented 4 years ago

I agree about not having per-request middleware—I think that should exclusively be a property of the client and if you want a different middleware stack you should use a different client instance.

Fishrock123 commented 4 years ago

I have added 8977cba49059e1762b191855ac9fba4bb845a0c4.

It adds client::{method}() as a different way to do client.send(req). I still don't like it but it seems to be the acceptable compromise that will allow the rest of this to move forward.

@yoshuawuyts, @jbr, @dignifiedquire