Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seanaye/feat/add misc methods #334

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

seanaye
Copy link
Contributor

@seanaye seanaye commented May 10, 2023

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
Copy link

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
Copy link

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
Copy link

#335 my current draft

@seanaye
Copy link
Contributor Author

seanaye commented May 11, 2023

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
Copy link

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
Copy link
Contributor Author

seanaye commented May 11, 2023

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
Copy link
Contributor Author

seanaye commented May 11, 2023

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
Copy link

Alextopher commented May 11, 2023

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
Copy link

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
Copy link
Contributor Author

seanaye commented May 11, 2023

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
Copy link
Contributor Author

seanaye commented May 11, 2023

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

@Alextopher
Copy link

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

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/net/src/http/headers.rs Outdated Show resolved Hide resolved
crates/net/src/http/response.rs Show resolved Hide resolved
@seanaye seanaye requested a review from ranile August 8, 2023 22:50
@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

@ranile
Copy link
Collaborator

ranile commented Aug 9, 2023

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.

@ranile ranile requested a review from futursolo August 9, 2023 18:51
@Alextopher
Copy link

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
Copy link
Collaborator

ranile commented Aug 9, 2023

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
Copy link

databasedav commented Sep 2, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants