Rework Client constructors
- Dominant language
- Rust
- Stars
- 1.5k
- Forks
- 128
- PR merge metrics
- No merged PRs in 30d
Description
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:
```rust
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](https://github.com/nodejs/undici) JS HTTP client always requires a base URL, and uses that instance to create all further requests from:
```js
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:
```rust
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:
```rust
pub fn with_http_client(http_client: Arc) -> 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:
```rust
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:
```rust
pub fn connect_with(http_client: C) -> crate::Result
where
C: HttpClient + Clone;
```
Which will enable us to write:
```rust
let mut app = tide::new();
let mut client = Client::connect_with("http://example.com", app)?;
Contributor guide
Assessment
This issue has not been assessed yet.