add surf::Methods

[jbr]

This PR adds a trait called surf::Methods which adds http methods directly to any http_client::HttpClient when the trait is in scope. In concert with http-rs/tide#697, this means that a minimal example for testing a tide app might be:

let app = tide::new();
use surf::ClientExt;
app.get("http://example.com/").recv_string()?;

This also just saves anyone else from having to type out all of the verbs — anyone that wants to implement this trait just needs to provide a fn client() -> Client and they get all of the one-off builders.

I'm not sure if this should be the only thing in a surf::prelude::*, but that makes sense to me as an extension trait

[Fishrock123]

I'm confused. Why does this need to exist? These methods already exist on Client: https://docs.rs/surf/2.0.0-alpha.5/surf/struct.Client.html

[Fishrock123]

It looks like this often may make something like Arc<Arc<dyn HttpClient>>. Maybe the signature of Client::with_http_client() needs to be improved.

Edit: Now that I look again I don't see it, maybe I was mistaken?

Turns out this adds these to any arbitrary HttpClient. I'm not quite sure what I think of it.

[Fishrock123]

Maybe this can be feature flagged while we figure out how we like it? Is that acceptable?

[yoshuawuyts]

I'm quite sold that this is a good idea. But what I'm less sure on is the name + location of the trait. We already have a surf::Client at the root; surf::ClientExt doesn't extend surf::Client so I can see that cause issues.

Crates like anyhow provide extension traits such as Context that are based on functionality rather than the *Ext scheme, and I feel we could perhaps do something similar. But really quite unsure about the naming...

[Fishrock123]

surf::HttpVerbs? I'm not really sure either.

[jbr]

Agreed on the name issue. MethodsExt? or does that sound too much like it extends http_types::Method?

[Fishrock123]

I think this could use better documentation about what exactly this is. I think that is my primary concern at this point, it may be confusing for someone to see a trait named "Methods" in the docs without more exposition.

[jbr]

Agreed, but not sure what to write. Suggestions appreciated

[yoshuawuyts]

Agreed on the name issue. MethodsExt? or does that sound too much like it extends http_types::Method?

Yeah, I think it might? Been re-revisiting this PR over the past few days, and that's def the feel I get.

Which constraints are we designing around?

Something I've still been thinking of is whether the ClientExt extension trait is actually needed. We're having a tough time naming it even though it's clear what we want to use it from: we want to convert a Tide server to a Surf client so that we can query it and validate the response.

This trait attempts to overcome two hurdles that are in converting Tide server -> Surf client:

1. Wrapping the Tide server in an Arc
2. Setting a default URL for Client

A solution to 1. could be to make it so each Client is responsible for implementing Clone themselves; that way we can rely on tide::Server's Clone impl without needing to wrap it in another Arc. Though this requires changes to http-client, it seems like a better design.

A solution for 2 would be for surf::Client to always require a base URL. This is how for example undici works, and it removes ambiguity from what can be passed when and where. This means constructing a Client would take the URL during construction.

Alternate API proposal

Putting these two together, we could imagine a new constructor method that could look like this:

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

This is slightly longer than the ClientExt-based one, but what we gain is control over the URLs used by the client -- which may become relevant when for example testing subdomains. Testing could then look something like this:

let mut app = tide::new();
app.subdomain("blog").at("/").get(|_| async { Ok("welcome to my blog") });
app.at("/index.html").get(|_| async { Ok("welcome to my site") };

let blog_client = Client::connect_with("https://blog.example.com", app.clone());
assert_eq!(blog_client.get("/").recv_string()?, "welcome to my blog".into()));

let client = Client::connect_with("https://example.com", app);
assert_eq!(client.get("/index.html").recv_string()?, "welcome to my site".into()));

Not being able to control the domain name in the client may actually also become an issue when testing other url-related things. So we should probably consider that as part of the design for a testing setup.

[Fishrock123]

This trait attempts to overcome two hurdles that are in converting Tide server -> Surf client:

2. Setting a default URL for Client

I'm really not sure where you are getting this from... neither this PR or that Tide PR do that directly in any way. That is functionality that only just so happens to also exist but is not really related?

I do not agree with making Client require a base url. That is not the only reason to use a Client, and it would be quite reasonable to make a URL-less client with some specific set of middleware and even then clone it and have other parts of a program use it as a base even if those parts end up adding the same or different base urls.

[Fishrock123]

I see this PR in particular as not really being fundamental in any way - it is purely ergonomics.

With http-rs/tide#697 you could just as well write this:

let server = tide::new();
let client = surf::Client::with_http_client(server);
client.get("http://example.com/").await?;

[jbr]

Exactly what @Fishrock123 said — this PR isn't all that important to the testing thing. I thought it would be convenient for "anyone who wants to add the full list of http verbs without having to copy and paste them" but maybe that's an infrequent enough problem that we should just close this issue and offer a tide-specific testing extension that does this specifically for tide::Servers. Unless someone objects, I'll write that up and close this PR