rustwasm
Seanaye/feat/add misc methods
Adds a number of utility methods that I have needed while working on a server integration
- Getting properties from the
ResponseBuilder - Access to the Response.clone javascript method MDN
- Clone trait for
ResponseBuilder
I've been dragging my feet on writing an RFC on these types. I'm a bit concerned with how Request doesn't follow typical ownership and borrowing rules (you only need a &T even when mutating) and I've noticed RequestBuilder is missing a lot of options. Here is how I've been thinking of improving this:
In order for a method like Request::url(&self) -> &String to exist we need something in rust to own the url in Rust. The same thing goes with the other getters. Instead of wrapping web_sys types, I think we should keep values in Rust as long as possible.
/// RequestOptions is used to create a [`web_sys::RequestInit`].
struct RequestOptions {
method: http::Method,
headers: http::HeaderMap, // or our current `Headers` type
body: Option<Body>,
cache: web_sys::RequestCache, // These web_sys types are enums
credentials: web_sys::RequestCredentials,
integrity: String,
mode: web_sys::RequestMode,
redirect: web_sys::RequestRedirect,
referrer: String,
referrer_policy: web_sys::ReferrerPolicy,
// signal: Option<&'a web_sys::AbortSignal>,
}
/// A convenient builder for [`Request`].
#[derive(Debug)]
#[must_use = "RequestBuilder does nothing unless you assign a body or call `build`"]
pub struct RequestBuilder {
url: String,
init: RequestOptions,
}
/// A wrapper around [`web_sys::Request`]. Requests are immutable after they are created.
#[derive(Debug)]
pub struct Request {
inner: web_sys::Request, // Only used to `send` and process the `body`
// Cached Rust representations of the request inners.
url: String,
init: RequestOptions,
}
I think this creates cleaner code: https://github.com/Alextopher/gloo/blob/http/crates/net/src/http/request.rs
The problem I see with my suggestion: it's a lot of breaking changes. However, I think it makes the library more mature and idiomatic and opens up more features that you previously had to use web_sys to use.
Could you please provide an example of where a mutation occurs with &T for the current code? I can't find anything other than reading the body which is a bit of a special case imo. I agree the builder needs the missing config fields. I looked at your PR I don't have any concerns other than breaking changes
I think methods that consume the body should capture ownership over self, reading the body twice (without cloning) is almost always bug.
But you're right my mutability concerns are mostly in QueryParams and Headers https://github.com/rustwasm/gloo/blob/master/crates/net/src/http/headers.rs#L61
I don't think its required to keep the init options inside of the Request. Its possible to keep it as struct Request(web_sys::Request) and use js_sys::Reflect::get to access the init options e.g. to access mode. Once the Request is built all properties are read only
Re: the duplicated reading, I think this is a matter of preference. I agree if the body was its own struct then reading it should take ownership but there is a lot of other data associated with the Request. It doesn't make sense to me that you cant read the url after reading the body because reading the body consumed the entire request.
I see that, I think there's a tradeoff:
Do we want this signature (which makes the returned value immutable)
pub fn integrity(&self) -> &String {
&self.options.integrity
}
Or is this good enough?
pub fn integrity(&self) -> String {
self.raw.integrity()
}
There's also a small time/space tradeoff. Using some extra space in wasm world can asymptomatically reduce the number of calls from wasm to js. What I've been able to do today : but I haven't pushed yet : is remove the the web_sys::Request from Request and only build it when needed (when calling send() or parsing the body).
#[derive(Debug)]
pub struct Request {
url: String,
options: RequestOptions,
}
impl From<web_sys::Request> for Request {
fn from(value: web_sys::Request) -> Self {
Self {
url: value.url(),
options: RequestOptions {
method: http::Method::from_bytes(value.method().as_bytes()).unwrap(),
headers: headers_from_js(&value.headers()),
body: value.body().map(Body::from),
cache: value.cache(),
credentials: value.credentials(),
integrity: value.integrity(),
mode: value.mode(),
redirect: value.redirect(),
referrer: value.referrer(),
referrer_policy: value.referrer_policy(),
},
}
}
}
impl TryFrom<Request> for web_sys::Request {
type Error = crate::Error;
fn try_from(value: Request) -> Result<Self, Self::Error> {
let init: web_sys::RequestInit = value.options.into();
web_sys::Request::new_with_str_and_init(value.url.as_str(), &init).map_err(js_to_error)
}
}
Re: the duplicated reading, I think this is a matter of preference. I agree if the body was its own struct then reading it should take ownership but there is a lot of other data associated with the Request.
I dislike how it's possible to read the body multiple times. In reqwest they use the same pattern https://docs.rs/reqwest/latest/reqwest/struct.Response.html#method.text for the same reasoning
I can think of changing the signature of send to return a tuple:
struct Body(web_sys::ReadableStream);
impl Body {
async fn text(self) -> Result<String>;
// etc
}
impl Request {
fn send(self) -> Result<(Response, Body)>
}
I think taking ownership is fine actually, just forces users to get what they want out of the response before reading the body. I will add a commit to take the ownership
Do you think fn body_used should be removed now? its not possible to read a body multiple times
I'm not sure - there's probably some value of it with regards to backwards compatibility, Or if someone is making a lot of use of into_raw() and from_raw(). I'd add a note that in normal operation it will return false
I dislike how it's possible to read the body multiple times. In
reqwestthey use the same pattern https://docs.rs/reqwest/latest/reqwest/struct.Response.html#method.text for the same reasoning
@Alextopher While I do think that doing it the reqwest way is good, it's not just body that can read. Taking ownership causes the entire object to be unusable later on. I would prefer to keep the ability to use the object, just as is possible in JS.
We could also split bodies like Tower (I think it's Tower I'll double check later). But the idea is splitting the Response into (Body, Parts) where Parts has all of the normal components.
I'm not an expert rust user but it feels un-rusty to leave a class of bug the type system could easily solve.
I'm not an expert rust user but it feels un-rusty to leave a class of bug the type system could easily solve.
I don't disagree. But at the same time, I also don't want to take away functionality that would otherwise be possible
could we add a feature flag that always .try_clone's the body when it's read? that way, we can preserve the references in the signatures, while avoiding the clone management stuff; or perhaps a separate auto-body-cloning struct?
