http-rs / http-rs/surf

Rework Client constructors

Open
#237 5 comments 0 reactions 0 assignees View on GitHub
semver-major
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.