`net` improve interoperability of wrapper types #332

Alextopher · 2023-05-01T21:44:07Z

`net` improve interoperability of wrapper types

This PR currently only covers the gloo_net crate. I'd be happy to (slowly) extend this idea to other crates if this is well received.

HTTP

`Request`

trait		reason
Clone	🟢	The original reason I started this PR. It can be useful to clone a `Request` to send it multiple times.
Copy	❌	The type is large (40 bytes) and copying should be explicit, and `web_sys` types don't implement `Copy`
PartialEq	🟢	It would be odd to check if you have two identical requests, but there's no reason to forbid it.
Eq	🟢	See `PartialEq`, reflexivity is guaranteed.
PartialOrd	❌	Not applicable.
Ord	❌	Not applicable.
Hash	❌	Inner web_sys types don't implement `Hash`
Debug	🟩	Existing.
Display	❌	Requests are fairly complex, perhaps a `Display` implementation could be implemented to follow the HTTP spec, but that's a larger discussion.
Default	❌	No obvious default value.

`Response`

Implement the same traits as the underlying web_sys type.

trait		reason
Clone	🟢	Added.
Copy	❌	Inner `web_sys` types don't implement `Copy`
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	❌	Not applicable.
Ord	❌	Not applicable.
Hash	❌	Inner web_sys types don't implement `Hash`
Debug	🟩	Existing.
Display	❌	Perhaps we could follow the HTTP spec?
Default	❌	No obvious default value.

Headers

Implement the same traits as the underlying web_sys type.

trait		reason
Clone	🟢	Added.
Copy	❌
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	❌
Ord	❌
Hash	❌
Debug	🟩	Existing.
Display	❌	Could be implemented to follow the HTTP spec.
Default	🟩	Existing.

`QueryParams`

Implement the same traits as the underlying web_sys type.

trait		reason
Clone	🟢	Added.
Copy	❌
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	❌
Ord	❌
Hash	❌
Debug	🟩	Existing.
Display	🟩	Existing.
Default	🟩	Existing.

`Method`

trait		reason
Clone	🟩	Existing.
Copy	🟩	Existing.
PartialEq	🟢	A simple Enum over the types of HTTP methods should be comparable.
Eq	🟢	Method::X == Method::X
PartialOrd	🟢	I've implemented the lexicographical ordering. I added a test against the `Display` implementation
Ord	🟢	See `PartialOrd`
Hash	🟢	A simple Enum is hashable.
Debug	🟩	Existing.
Display	🟩	Existing.
Default	❌	Not applicable.

Added as_str method to Method to get a &'static str representation of the method. This avoids some unnecessary allocations.

eventsource

`EventSource`

Implement the same traits as the underlying web_sys type.

trait		reason
Clone	🟩	Existing.
Copy	❌
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	❌
Ord	❌
Hash	❌
Debug	🟩	Existing.
Display	❌

`EventSourceSubscription`

I don't think any traits are applicable here. Closure prevents most traits from being implemented.

trait		reason
Debug	🟩	Existing.

`eventsource::State`

In JS the readyState property returns a Number 0, 1, or 2 (connecting, open, closed). I've implemented the expected traits for this type.

trait		reason
Clone	🟩	Existing.
Copy	🟩	Existing.
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	🟢	Added.
Ord	🟢	Added.
Hash	🟢	Added.
Debug	🟩	Existing.
Display	🟢	"connecting", "open", "closed"

Added as_str to get a &'static str representation of the state. This avoids some unnecessary allocations.

Added From<u16> for State and From<State> for u16 to convert between the JS number and the Rust enum.

`EventSourceError`

I've left this trait unchanged, since its non_exhaustive I'm not confident if it will remain as simple as it is now. If it does remain "simple" I'd suggest implementing Copy and Hash.

trait		reason
Clone	🟩	Existing.
Copy	⚠️	If this type will remain simple then it should become copyable.
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	❌	Error types aren't orderable
Ord	❌
Hash	⚠️	If this type will remain simple then it should become hashable.
Debug	🟩	Existing.
Display	🟩	Existing.

Ord doesn't make sense for an error type.

websocket

`CloseEvent`

trait		reason
Clone	🟩	Existing.
Copy	❌	32 bytes is a bit large for `Copy`
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	❌
Ord	❌
Hash	🟢	Added.
Debug	🟩	Existing.
Display	❌	I think individual users will have differing preferences for how to display this type.

`WebSocket`

No traits are applicable here. The underlying types prevent most traits from being implemented. Particularly the Closure fields.

`Message`

trait		reason
Clone	🟩	Existing.
Copy	❌	`String` and `Vec` aren't `Copy`
PartialEq	🟢	Existing.
Eq	🟩	Existing.
PartialOrd	❌	Not applicable.
Ord	❌	Not applicable.
Hash	🟢	Added.
Debug	🟩	Existing.
Display	❌	I think individual users will have differing preferences for how to `Display` a `Vec<u8>`

`websocket::State`

Similar to eventsource::State.

In JS the readyState property returns a Number (0, 1, 2, 3) (connecting, open, closing, closed). I've implemented the expected traits for this type.

trait		reason
Clone	🟩	Existing.
Copy	🟩	Existing.
PartialEq	🟢	Added.
Eq	🟢	Added.
PartialOrd	🟢	Added, follows the ordering of connection lifecycle
Ord	🟢	Added, follows the ordering of connection lifecycle
Hash	🟢	Added.
Debug	🟩	Existing.
Display	🟢	"connecting", "open", "closing", "closed"

Added as_str to get a &'static str representation of the state. This avoids some unnecessary allocations.

Added From<u16> for State and From<State> for u16 to convert between the JS number and the Rust enum.

`WebSocketError`

This enum depends on JsError which does not implement many traits. If that changes then this type should implement more traits.

trait		reason
Clone	❌	`JsError` doesn't implement `Clone`
Copy	❌	`JsError` doesn't implement `Copy`
PartialEq	❌	`JsError` doesn't implement `PartialEq`
Eq	❌	`JsError` doesn't implement `Eq`
PartialOrd	❌	Error types aren't orderable
Ord	❌
Hash	❌	`JsError` doesn't implement `Hash`
Debug	🟩	Existing.
Display	🟩	Existing.

net

`Error`

Same as WebSocketError, JsError hasn't been covered by this PR. Once JsError implements more traits this type should implement more traits.

trait		reason
Debug	🟩	Existing.

futursolo

Thank you for the pull request. I have only reviewed the first section (http) as the master branch has conflicts with this pull request.

I do wish to provide some of my personal opinion on the first part of the implementation which I think is applicable for the rest of the pull request as well.

Feel free to request another review after you have resolved the conflicts.

Request

Trait	My Opinion	Reason
Clone	❌	I do not think `web_sys::RequestInit` is clonable when `web_sys::ReadableStream` is used as body. We shouldn't provide an implementation that could panic from time to time.
PartialEq	❌	web_sys delegates them to `a == b` in JavaScript and not trying to check whether the underlying value is equal. I do not think this provides the desired behaviour when `==` is used in Rust where users would expect their internal values to equal. I do not think a sound `PartialEq` and `Eq` implementation is possible with `web_sys::RequestInit` as you cannot compare `web_sys::ReadableStream` if it is used as body for one or both requests used in the comparison.
Eq	❌	See above

Response

Similar to request.

Headers

Trait	My Opinion	Reason
Clone	⚠️	I think this can be provided. However, we should not use the web_sys's Clone implementation. web_sys only increases the reference count of the same `Headers` JavaScript instance and not actually cloning the value into a different `Header()`. This would cause the cloned headers instance share any changes applied on the original header instance after the clone. If you wish to implement Clone, you need to implement them manually with `Headers::new_with_headers()`.
PartialEq	⚠️	This is similar to request where `a == b` does not provide desirable result, but I think it's not as difficult to provide a manual implementation. If you wish to implement PartialEq, you need to implement the trait manually.
Eq	⚠️	See above

QueryParams

Similar to Headers.

Method

We switched to Method type from the http crate. This is now only a type alias.

Legend

❌: I do not think a sound implementation is possible.
⚠️: I think a sound implementation is possible but we should do something else than what web_sys is doing.

Alextopher · 2023-05-02T12:34:30Z

Just so that I understand, the web_sys #[derive(Clone)] macro is not the same as calling the object's clone method in JS (for example https://fetch.spec.whatwg.org/#dom-request-clone)?

So to improve this we either

Need to write our own way of calling clone (using wasm_bindgen) and then write a safe wrapper (something like TryClone) to catch the TypeError
Write a competing clone method that "builds" identical objects through the existing web_sys bindings

futursolo · 2023-05-02T13:26:11Z

Just so that I understand, the web_sys #[derive(Clone)] macro is not the same as calling the object's clone method in JS (for example https://fetch.spec.whatwg.org/#dom-request-clone)?

In this case, the Request struct holds a web_sys::RequestInit instead of web_sys::Request. web_sys::RequestInit is the RequestInit dictionary described in the spec, which should be a plain JavaScript object that doesn't have a clone method.

The derived Clone method for RequestInit eventually gets delegated to wasm_bindgen::JsValue which then simply increases the object reference count in a special array used by wasm_bindgen to access JavaScript values.

See: https://rustwasm.github.io/wasm-bindgen/api/src/wasm_bindgen/lib.rs.html#1091-1099

Need to write our own way of calling clone (using wasm_bindgen) and then write a safe wrapper (something like TryClone) to catch the TypeError
Write a competing clone method that "builds" identical objects through the existing web_sys bindings

I am fine with both ways as long as it does not significantly increase the bundle size.
(I am also fine with doing away with the clone implementation completely if it ends up too complicated.)

However, I do not think a non-fallible clone is possible as web_sys::ReadableStream is not clonable and any requests that use it as body will in turn become unclonable.

In the latest master branch, the Request struct is separated into Request and RequestBuilder, it might be easier if only implementing cloning on RequestBuilder as the builder does not contain a body?

Alextopher · 2023-05-02T16:04:11Z

I think cloning a request & response could be made infallible if the other apis were more rust idiomatic.

For example: Response::text takes a &self even though it consumes the underlying body, so repeated calls to text will always error, and we can't clone. I think the more rust idiomatic way is to have text take ownership of the response (reqwest).

ranile

Thanks for the PR!

This was mostly oversight when implementing the types. Thanks for addressing it. ~~In addition to what futursolo has mentioned, I have one question.~~ nevermind... it seems they mentioned it as well

crates/net/src/http/headers.rs

Alextopher · 2023-05-02T21:33:19Z

crates/net/src/http/headers.rs

+    /// assert_eq!(headers.get("Content-Type"), Some("text/plain, text/html".to_string()));
+    /// # }
+    /// ```
+    pub fn append(&mut self, name: &str, value: &str) {


I know web_sys::Headers (and all? web_sys types) allow inner mutability through a &T but I'm not sure if it's idiomatic to do the same on our wrapper types.

For example, if someday we choose to change the implementation of Header wrap a HashSet<&str, String> this method would need to be &mut or we're committing to using a something like RefCell<_> forever

Alextopher · 2023-05-02T21:34:01Z

crates/net/src/http/headers.rs

+    }
+}
+
+impl<K, V> Extend<(K, V)> for Headers


https://rust-lang.github.io/api-guidelines/interoperability.html#collections-implement-fromiterator-and-extend-c-collect

Alextopher · 2023-05-02T21:35:29Z

crates/net/src/http/request.rs

@@ -38,10 +38,9 @@ impl RequestBuilder {
    }

    /// Set the body for this request.
-    pub fn body(mut self, body: impl Into<JsValue>) -> Result<Request, Error> {
+    pub fn body(mut self, body: impl Into<JsValue>) -> Self {


I didn't see a reason why Request body should build , this allows us to set the body in any order.

I will add a test to verify this is sound.

Alextopher · 2023-05-02T21:37:50Z

crates/net/src/http/response.rs

+    /// # Errors
+    ///
+    /// This method may return a "RangeError"
+    pub async fn binary(self) -> Result<Vec<u8>, Error> {


Methods that consume Request/Response bodies now take ownership of self. This makes clone infallible and prevents the class of bugs where users read the body multiple times.

Alextopher · 2023-05-02T21:38:17Z

I haven't gone back to websocket or eventsource yet.

futursolo · 2023-05-03T05:03:23Z

After re-examining the updated pull request, I think some of criteria has exceeded simply adding additional traits and I think is kind of difficult to review them with all the changes mixed in the same PR.

Could you please split the pull request into multiple smaller ones and limit to adding traits?

I think the some of API changes contained in this pull request to certain types are in the right direction but significant. If you wish to change them, would you mind start with an RFC?

Alextopher · 2023-05-10T13:16:32Z

(draft) RFC: #335 I agree the changes I've suggested have surpassed what this PR was originally for. I think it's worth closing 😃

Alextopher added 5 commits May 1, 2023 17:33

net improve interoperability of wrapper types

af3e483

to and From<u16> for websocket::State

d8f816f

improve tests for Ord types

febbb20

nit: fix formatting error

ab32562

add Ord warning to eventsource::State

8b77934

futursolo requested changes May 2, 2023

View reviewed changes

Merge branch 'master' into c-common-traits

9efe07a

ranile reviewed May 2, 2023

View reviewed changes

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

Alextopher commented May 2, 2023

View reviewed changes

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

Alextopher added 2 commits May 2, 2023 17:25

make function signatures in http more idiomatic

6c32898

nit: fmt

61d9685

Alextopher commented May 2, 2023

View reviewed changes

Alextopher closed this May 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`net` improve interoperability of wrapper types #332

`net` improve interoperability of wrapper types #332

Alextopher commented May 1, 2023 •

edited

Loading

futursolo left a comment •

edited

Loading

Alextopher commented May 2, 2023

futursolo commented May 2, 2023 •

edited

Loading

Alextopher commented May 2, 2023

ranile left a comment •

edited

Loading

Alextopher May 2, 2023 •

edited

Loading

Alextopher May 2, 2023

Alextopher May 2, 2023

Alextopher May 2, 2023

Alextopher commented May 2, 2023

futursolo commented May 3, 2023 •

edited

Loading

Alextopher commented May 10, 2023

net improve interoperability of wrapper types #332

net improve interoperability of wrapper types #332

Conversation

Alextopher commented May 1, 2023 • edited Loading

net improve interoperability of wrapper types

HTTP

Request

Response

Headers

QueryParams

Method

eventsource

EventSource

EventSourceSubscription

eventsource::State

EventSourceError

websocket

CloseEvent

WebSocket

Message

websocket::State

WebSocketError

net

Error

futursolo left a comment • edited Loading

Choose a reason for hiding this comment

Request

Response

Headers

QueryParams

Method

Legend

Alextopher commented May 2, 2023

futursolo commented May 2, 2023 • edited Loading

Alextopher commented May 2, 2023

ranile left a comment • edited Loading

Choose a reason for hiding this comment

Alextopher May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Alextopher May 2, 2023

Choose a reason for hiding this comment

Alextopher May 2, 2023

Choose a reason for hiding this comment

Alextopher May 2, 2023

Choose a reason for hiding this comment

Alextopher commented May 2, 2023

futursolo commented May 3, 2023 • edited Loading

Alextopher commented May 10, 2023

`net` improve interoperability of wrapper types #332

`net` improve interoperability of wrapper types #332

Alextopher commented May 1, 2023 •

edited

Loading

`net` improve interoperability of wrapper types

`Request`

`Response`

`QueryParams`

`Method`

`EventSource`

`EventSourceSubscription`

`eventsource::State`

`EventSourceError`

`CloseEvent`

`WebSocket`

`Message`

`websocket::State`

`WebSocketError`

`Error`

futursolo left a comment •

edited

Loading

futursolo commented May 2, 2023 •

edited

Loading

ranile left a comment •

edited

Loading

Alextopher May 2, 2023 •

edited

Loading

futursolo commented May 3, 2023 •

edited

Loading