Rework Client constructors

[yoshuawuyts]

This is a follow-up to #229 (comment), 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 http-rs/tide#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]

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]

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. (crate: add hyper-client feature #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]

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: #238

[yoshuawuyts]

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

[Fishrock123]

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