Seanaye/feat/add misc methods #334

seanaye · 2023-05-10T03:44:14Z

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

Alextopher · 2023-05-10T12:09:11Z

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

Alextopher · 2023-05-10T12:12:36Z

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.

Alextopher · 2023-05-10T12:31:32Z

#335 my current draft

seanaye · 2023-05-11T01:14:43Z

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

Alextopher · 2023-05-11T01:19:52Z

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

seanaye · 2023-05-11T01:23:19Z

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

seanaye · 2023-05-11T01:31:49Z

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.

Alextopher · 2023-05-11T01:32:26Z

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)
    }
}

Alextopher · 2023-05-11T01:48:26Z

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)> 
}

seanaye · 2023-05-11T02:03:04Z

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

seanaye · 2023-05-11T02:21:47Z

Do you think fn body_used should be removed now? its not possible to read a body multiple times

Alextopher · 2023-05-11T20:54:46Z

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

ranile

Thanks for the PR! There's a couple of comments that need to be addressed

crates/net/src/http/headers.rs

crates/net/src/http/response.rs

…oo into seanaye/feat/add-misc-methods

ranile · 2023-08-09T18:39:59Z

crates/net/src/http/headers.rs

@@ -37,14 +44,14 @@ impl Headers {

    /// This method appends a new value onto an existing header, or adds the header if it does not
    /// already exist.
-    pub fn append(&self, name: &str, value: &str) {
+    pub fn append(&mut self, name: &str, value: &str) {


I'm not a fan of making this &mut. This makes the method unnecessarily restrictive.

CC: @futursolo, do you have any thoughts on this?

ranile · 2023-08-09T18:44:58Z

crates/net/src/http/request.rs

@@ -265,12 +265,12 @@ impl Request {
    }

    /// Gets the body.
-    pub fn body(&self) -> Option<ReadableStream> {
+    pub fn body(self) -> Option<ReadableStream> {


This function can be called multiple times and still be valid.

ranile · 2023-08-09T18:45:32Z

crates/net/src/http/request.rs

        serde_json::from_str::<T>(&self.text().await?).map_err(Error::from)
    }

    /// Reads the reqeust as a String.
-    pub async fn text(&self) -> Result<String, Error> {
+    pub async fn text(self) -> Result<String, Error> {


This change, while good in theory, also makes the API unnecessarily restrictive.

Notice how the object can still be used after text() is called

ranile · 2023-08-09T18:51:21Z

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

@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.

Alextopher · 2023-08-09T22:17:05Z

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.

ranile · 2023-08-09T22:18:49Z

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

databasedav · 2023-09-02T04:40:01Z

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?

seanaye added 4 commits May 9, 2023 23:28

add getter methods and impl clone responsebuilder

093a61f

expose responsebuilder

4278e9a

add try clone method

d450642

cargo fmt

9233fa4

seanaye added 3 commits May 10, 2023 22:04

take ownership of request when reading body

ca68183

take ownership of response when reading body

3af657d

fix tests

d6ea45a

derive clone headers

63b98b7

ranile requested changes Jul 24, 2023

View reviewed changes

crates/net/src/http/headers.rs Outdated Show resolved Hide resolved

crates/net/src/http/response.rs Show resolved Hide resolved

seanaye added 4 commits August 1, 2023 23:38

improve safety and clone headers

52f1a5d

Merge branch 'master' into seanaye/feat/add-misc-methods

6a89f16

run cargo fmt

cf92a0b

Merge branch 'seanaye/feat/add-misc-methods' of github.com:seanaye/gl…

1c3e67a

…oo into seanaye/feat/add-misc-methods

seanaye requested a review from ranile August 8, 2023 22:50

ranile reviewed Aug 9, 2023

View reviewed changes

ranile requested a review from futursolo August 9, 2023 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seanaye/feat/add misc methods #334

Seanaye/feat/add misc methods #334

seanaye commented May 10, 2023

Alextopher commented May 10, 2023

Alextopher commented May 10, 2023

Alextopher commented May 10, 2023

seanaye commented May 11, 2023

Alextopher commented May 11, 2023

seanaye commented May 11, 2023

seanaye commented May 11, 2023

Alextopher commented May 11, 2023 •

edited

Loading

Alextopher commented May 11, 2023

seanaye commented May 11, 2023

seanaye commented May 11, 2023

Alextopher commented May 11, 2023

ranile left a comment

ranile Aug 9, 2023

ranile Aug 9, 2023

ranile Aug 9, 2023

ranile commented Aug 9, 2023

Alextopher commented Aug 9, 2023

ranile commented Aug 9, 2023

databasedav commented Sep 2, 2023 •

edited

Loading

Seanaye/feat/add misc methods #334

Are you sure you want to change the base?

Seanaye/feat/add misc methods #334

Conversation

seanaye commented May 10, 2023

Alextopher commented May 10, 2023

Alextopher commented May 10, 2023

Alextopher commented May 10, 2023

seanaye commented May 11, 2023

Alextopher commented May 11, 2023

seanaye commented May 11, 2023

seanaye commented May 11, 2023

Alextopher commented May 11, 2023 • edited Loading

Alextopher commented May 11, 2023

seanaye commented May 11, 2023

seanaye commented May 11, 2023

Alextopher commented May 11, 2023

ranile left a comment

Choose a reason for hiding this comment

ranile Aug 9, 2023

Choose a reason for hiding this comment

ranile Aug 9, 2023

Choose a reason for hiding this comment

ranile Aug 9, 2023

Choose a reason for hiding this comment

ranile commented Aug 9, 2023

Alextopher commented Aug 9, 2023

ranile commented Aug 9, 2023

databasedav commented Sep 2, 2023 • edited Loading

Alextopher commented May 11, 2023 •

edited

Loading

databasedav commented Sep 2, 2023 •

edited

Loading