Rework Client constructors

yoshuawuyts commented 3 years ago

This is a follow-up to https://github.com/http-rs/surf/pull/229#issuecomment-695781065, breaking the proposal there into smaller issues. The goal of this is to enable this to work:

let app = tide::new();
let client = Client::connect_with("https://example.com", app)?;
let output = client.get("/").recv_string().await?;

I think this may also serve as an alternative to https://github.com/http-rs/tide/pull/701.

Client should always have a base url

Right now Client::get can either take a fully qualified URL or a path, depending on whether a base URL was configured. This degree of flexibility seems somewhat confusing, and setting the base URL takes some work to do. In contrast the undici JS HTTP client always requires a base URL, and uses that instance to create all further requests from:

const { Client } = require('undici')
const client = new Client(`http://example.com`)

client.request({ path: '/', method: 'GET' }, (err, data) => {
  if (err) throw err
  console.log('response received', data.statusCode)
  client.close()
})

I think this model makes it easier to reason about how pooling works, how URLs are constructed, and enables streamlining the constructor too. Instead of making it so setting a base URL takes two lines, we could require it be part of the constructor:

use surf::Client;

let client = Client::new("https://example.com")?;
let res = client.get("/").await?;
println!("response received {}", res.status());

Renaming of constructor methods?

Something I've been toying with is: what if we renamed the Client constructor methods. In particular Client::with_http_client doesn't really roll of the tongue. Instead what I've been toying with is:

Client::connect(url) to construct a new client -- if we make this fallible + async this could be a logical place to e.g. construct a threadpool.
Client::connect_with(url, client) to construct a new client with a backing impl. If we could construct a backing impl from a closure (perhaps in the future?) then the _with suffix would be a perfect fit. But if not that's also ok.

One downside of this is that there's a CONNECT HTTP method too; so we probably couldn't expose these from the crate root. But I don't see that as too big of a downside.

HttpClient instances should be Clone

Right now the Client::with_http_client method has the following signature:

pub fn with_http_client(http_client: Arc<dyn HttpClient>) -> Self;

This means that even if a backend we pass implements Clone, we must wrap it in another Arc. This leads to constructs such as:

let mut app = tide::new();
let mut client = Client::with_http_client(Arc::new(app));
client.set_base_url("http://example.com/");

Instead I'd like us to change the signature to:

pub fn connect_with<C>(http_client: C) -> crate::Result<Self>
where
    C: HttpClient + Clone;

Which will enable us to write:


let mut app = tide::new();
let mut client = Client::connect_with("http://example.com", app)?;

Fishrock123 commented 3 years ago

~~The HttpClient thing is, frankly, largely separate and can be done already, separately from the concerns of the rest.~~

This isn't really true, because we can't have it be Clone because it is a Trait object... But also I don't know how we would make that change without reintroducing a generic bound to Client which I'd really like to avoid?

Edit... see latest associated comments and PRs...

Fishrock123 commented 3 years ago

For the rest, I think that would necessitate a ClientBuilder ...

Stated again, a base URL is not the only reason to use a Surf client:

Pooling already exists in the Isahc client without any modifications.
Pooling will exist in the Hyper client, without any API modifications. (https://github.com/http-rs/surf/pull/234)
One may want to have a client with a base set of middleware that is cloned and used to base other clients on.

Fwiw: Hyper also uses a builder for it's client (with far more fine-grained options).

Fishrock123 commented 3 years ago

I've made a PR for

pub fn with_http_client<C: HttpClient>(http_client: C) -> Client

since that seems easy enough to do and I'm pretty sure the existing public Arc-taking one nobody liked: https://github.com/http-rs/surf/pull/238

yoshuawuyts commented 3 years ago

Link to Hyper's builder: https://docs.rs/hyper/0.13.8/hyper/client/struct.Builder.html

Fishrock123 commented 3 years ago

The more I think about this, the better of an idea it seems. I propose we do the following:

Client::new(AsRef<str>)
Client::default() where no internal base url is set.
client.{method}(AsRef<str>) which panics if the client is missing a base url.
client.send(req) unchanged from today, i.e. will not enforce a base url on a full-url request

http-rs / surf