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

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

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

Conversation

indietyp
Copy link

Description

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

  1. WebContext has a new Unknown variant, which is only initialized if setInterval and clearInterval are available
  2. 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
Copy link
Author

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.

@indietyp indietyp changed the title Enable websocket-websys in NodeJS environments. feat: enable websocket-websys in NodeJS environments. Dec 22, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

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

I've left some feedback!

transports/websocket-websys/src/lib.rs Outdated Show resolved Hide resolved
transports/websocket-websys/CHANGELOG.md Outdated Show resolved Hide resolved
transports/websocket-websys/src/lib.rs Outdated Show resolved Hide resolved
transports/websocket-websys/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 69 to 76
/// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this just an enum?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, lets document this please.

Comment on lines 65 to 67
fn has(value: &JsValue, key: &str) -> bool {
js_sys::Reflect::has(value, &JsValue::from_str(key)).unwrap_or(false)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

///
/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
	},
	// ...
}

Opaque(JsValue),
}

// we're using additional indirection here, to prevent the caller from knowing
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! A few more suggestions :)

## 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +329 to +330
100, /* Chosen arbitrarily and likely worth tuning. Due to low impact of the /ws
* transport, no further effort was invested at the time. */
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +105 to +109
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."
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

mergify bot commented Apr 15, 2024

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

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.

websocket_websys does not work in NodeJS environments
2 participants