feat: enable `websocket-websys` in NodeJS environments. #5025

indietyp · 2023-12-22T09:14:39Z

Description

This enables websocket-websys in NodeJS environments through the following mechanisms:

WebContext has a new Unknown variant, which is only initialized if setInterval and clearInterval are available
dial() will panic if WebSocket is unavailable. To circumvent any speed issues, the value is cached in an atomic and retrieved when asked multiple times.

(2) is essential, as otherwise, we try to access an object that doesn't exist; from my experience, this will make NodeJS do weird things, halting execution immediately of the whole script, returning success, and swallowing any error.

Fixes #5024

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

(3) is a bit hard here, as far as I know we don't yet really have a test harness for both web workers and nodejs environments, is that correct?

indietyp · 2023-12-22T09:16:36Z

This also fixes another bug that was present (unintentionally?) where the code would correctly detect the worker scope but then refer to the globals instead of the worker scope.

thomaseizinger

Sorry for the long delay, I was on vacation :)

I've left some feedback!

transports/websocket-websys/src/lib.rs

transports/websocket-websys/CHANGELOG.md

transports/websocket-websys/src/lib.rs

thomaseizinger · 2024-01-15T01:35:18Z

transports/websocket-websys/src/web_context.rs

+/// A wrapper around a `u8` that is used to indicate whether the `WebSocket`
+/// API is supported.
+///
+/// * 0x00: Not yet checked
+/// * 0x01: Supported
+/// * 0x02: Not supported
+/// * other: Unknown -> fallback to `0x00`
+struct WebSocketSupport(AtomicU8);


Why isn't this just an enum?

Using an AtomicU8 instead allows us to load/store on &self, making it suitable for static variable use.

I see, lets document this please.

thomaseizinger · 2024-01-15T01:35:48Z

transports/websocket-websys/src/web_context.rs

+fn has(value: &JsValue, key: &str) -> bool {
+    js_sys::Reflect::has(value, &JsValue::from_str(key)).unwrap_or(false)
+}


Please move utility functions below the public API of the module.

transports/websocket-websys/src/web_context.rs

thomaseizinger · 2024-01-15T01:39:39Z

transports/websocket-websys/src/web_context.rs

+    ///
+    /// In particular this happens when one tries to call `clear_interval` on a
+    /// handle that was created in an unknown context in a known context.
+    pub(crate) fn clear_interval(&self, handle: IntervalHandle) {
        match self {


I think this would be cleaner if you'd match on the tuple:

match (self, handle) { (Self::Window(window), IntervalHandle(HandleValue::Numeric(i))) => { ... }, // ... }

not necessarily, we don't know what the handle is and the JsValue returned might also be an i32, in that case it is totally valid to give it to the window or worker as well.

Really? Isn't this error case somewhat impossible? The JS environment shouldn't suddenly change and return numbers instead of objects.

If we'd have dependent types then this would be type-safe, right?

Yes via dependent types this would be type-safe, but just to be a bit more on the edge of caution. I could imagine an (unlikely) case where someone changes the globals, e.g., from the nodejs ones to the browser or vice versa; in that case, we wouldn't recover. While the possibility for this is not high, I thought if we can, we can build in some additional resiliency.

Okay fair! I think it would still be cleaner to do something like the following then:

match (self, handle.into_i32()) { (Self::Window(window), Some(handle)) => { window.clear_interval_with_handle(handle); }, // ... }

thomaseizinger · 2024-01-15T01:40:09Z

transports/websocket-websys/src/web_context.rs

+    Opaque(JsValue),
+}
+
+// we're using additional indirection here, to prevent the caller from knowing


Everything is pub(crate) here so I don't think we need to be this vigilant :)

…into bm/nodejs-interval

thomaseizinger

Thanks! A few more suggestions :)

thomaseizinger · 2024-01-17T03:18:12Z

transports/websocket-websys/CHANGELOG.md

+## 0.3.2
+
+- Add support for environments which globally expose `setInterval` and `clearInterval` instead of a window.
+- This crate will now correctly panic if it cannot find the `WebSocket` global JavaScript object.


Suggested change

- This crate will now correctly panic if it cannot find the `WebSocket` global JavaScript object.

This crate will now correctly panic if it cannot find the `WebSocket` global JavaScript object.

Plus missing PR link.

thomaseizinger · 2024-01-17T03:18:51Z

transports/websocket-websys/src/lib.rs

+                100, /* Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws
+                      * transport, no further effort was invested at the time. */


Suggested change

100, /* Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws

* transport, no further effort was invested at the time. */

100, // Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws transport, no further effort was invested at the time.

thomaseizinger · 2024-01-17T03:23:33Z

transports/websocket-websys/src/lib.rs

+        if !WebContext::is_websocket_supported() {
+            panic!(
+                "Websockets are not supported in this environment, you can use the `isomorphic-ws` npm package to polyfill `global.WebSocket` to enable support."
+            );
+        }


We can't panic here, this is bad library behaviour! I'd suggest that instead of interacting with WebSocket directly here, we create an API on WebContext that makes a new connection, something like:

impl WebContext { fn dial(&self, url: String) -> Result<Connection, ...> { } }

Note the &self. This forces the transport module to go through WebContext::new which already checks that websockets are supported in this environment. If it returns None you can return an Error here that fails the DialFuture with a message that websockets are not supported.

mergify · 2024-04-15T09:09:18Z

This pull request has merge conflicts. Could you please resolve them @indietyp? 🙏

indietyp added 5 commits December 22, 2023 09:47

feat: make WebContext aware of Node environments

805931a

feat: take handle instead of copy

8a61f83

feat: unclone the handle

eb2b0cb

feat: panic if websockets unavailable

3952387

docs: NodeJS notes

61a0868

indietyp changed the title ~~Enable websocket-websys in NodeJS environments.~~ feat: enable websocket-websys in NodeJS environments. Dec 22, 2023

Merge branch 'master' into bm/nodejs-interval

5680438

thomaseizinger reviewed Jan 15, 2024

View reviewed changes

indietyp added 5 commits January 15, 2024 10:09

chore: remove accidental formatting

289b0d6

feat: apply suggestions from code review

a335ef9

chore: bump version

fbf4a49

chore: bump version (II)

89b22fc

Merge branch 'bm/nodejs-interval' of github.com:indietyp/rust-libp2p …

dd08226

…into bm/nodejs-interval

thomaseizinger reviewed Jan 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable `websocket-websys` in NodeJS environments. #5025

feat: enable `websocket-websys` in NodeJS environments. #5025

indietyp commented Dec 22, 2023

indietyp commented Dec 22, 2023

thomaseizinger left a comment

thomaseizinger Jan 15, 2024

indietyp Jan 15, 2024

thomaseizinger Jan 15, 2024

thomaseizinger Jan 15, 2024

thomaseizinger Jan 15, 2024

indietyp Jan 15, 2024

thomaseizinger Jan 15, 2024

indietyp Jan 15, 2024

thomaseizinger Jan 17, 2024

thomaseizinger Jan 15, 2024

thomaseizinger left a comment

thomaseizinger Jan 17, 2024

thomaseizinger Jan 17, 2024

thomaseizinger Jan 17, 2024

mergify bot commented Apr 15, 2024

	- This crate will now correctly panic if it cannot find the `WebSocket` global JavaScript object.
	This crate will now correctly panic if it cannot find the `WebSocket` global JavaScript object.

		100, /* Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws
		* transport, no further effort was invested at the time. */

	100, /* Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws
	* transport, no further effort was invested at the time. */
	100, // Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws transport, no further effort was invested at the time.

feat: enable websocket-websys in NodeJS environments. #5025

Are you sure you want to change the base?

feat: enable websocket-websys in NodeJS environments. #5025

Conversation

indietyp commented Dec 22, 2023

Description

Change checklist

indietyp commented Dec 22, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Apr 15, 2024

feat: enable `websocket-websys` in NodeJS environments. #5025

feat: enable `websocket-websys` in NodeJS environments. #5025