http-rs / surf

Fast and friendly HTTP client framework for async Rust
https://docs.rs/surf
Apache License 2.0
1.45k stars 119 forks source link

Fix client base URL documentation #252

Closed CrockAgile closed 3 years ago

CrockAgile commented 3 years ago

The client base URL example sets the base URL to "http://example.com/api/v1", which is then joined with a URI of "/posts.json". This had two bugs:

This commit fixes both of these bugs in the example. It also adds a note to the documentation (copied from the URL crate) about the importance of the ending '/'.

One test was added to ensure the base_url behavior.

jbr commented 3 years ago

Thanks for catching this! My inclination would be to treat this as a bug in the code instead of poor documentation. Is there any reason the trailing slash should be important? We should add one if the base url doesn’t end with /.

I’m not as sure about the leading slash, but my inclination would also be to strip that. It seems less surprising

CrockAgile commented 3 years ago

Yeah, this being a bug is a possible interpretation! I'm by no means an expert on the WHATWG URL standard tho, and this behavior arises from the canonical servo/rust-url crate. Reading over some of their issues, it seems this kind of behavior is a little confusing/ambiguous.

The URL crate was made for use in servo, and it seems both Chrome and Firefox follow this behavior:

// base URL missing trailing slash
(new URL('posts.json', 'http://example.com/api/v1')).toString()
"http://example.com/api/posts.json"

// relative path beginning with slash
(new URL('/posts.json', 'http://example.com/api/v1/')).toString()
"http://example.com/posts.json"

// "intended" result
(new URL('posts.json', 'http://example.com/api/v1/')).toString()
"http://example.com/api/v1/posts.json"

So if that's how joining URL and a path "should" work, what options does surf have?

CrockAgile commented 3 years ago

The "???" was meant to leave room for any other proposals anyone has, so please speak up if you see a good way to handle this 🙌

Fishrock123 commented 3 years ago

I've created an issue for this at https://github.com/http-rs/surf/issues/255

I'm going to merge this as a stop-gap for now.