websocket_websys does not work in NodeJS environments

[indietyp]

Summary

I am currently trying to write a module that relies on libp2p with a websocket connection between the client (uses WASM) and server (uses normal Rust), while doing so I have realized that the websocket_websys package seems to be incompatible with the nodejs target, considering that WebSocket is not natively available. I receive the error OutgoingConnectionError { connection_id: ConnectionId(1), peer_id: None, error: Transport([("/ip4/127.0.0.1/tcp/4088/ws", Other(Custom { kind: Other, error: Other(Right(Left(Left(Error { msg: "Invalid websocket url: ws://127.0.0.1:4088/" })))) }))]) }. I believe this is because the API simply doesn't exist.

Trying to crudely shim API does not help. Any help would be appreciated (or alternative nodejs compatible transports)

Expected behavior

Able to connect via websockets between server and client.

Actual behavior

Error out when trying to import with WASM seemingly crashing (?)

Relevant log output

No response

Possible Solution

Referencing the ws crate instead of WebSocket would be a possibility (or isomorphic-ws), but that would require external dependencies.

Version

0.53.2

Would you like to work on fixing this bug ?

Yes

[indietyp]

Shiming with isomorphic-ws I get panicked at [...]/libp2p-websocket-websys-0.3.1/src/lib.rs:308:14: to have a window or worker context

[indietyp]

I continued digging and found that the "polyfill" worked. This is no real solution, but it seems not as hard to support as expected.

// @ts-ignore
global.WebSocket = require('isomorphic-ws');
const globalSetInterval = global.setInterval;
// @ts-ignore
global.setInterval = (...args) => {
    // @ts-ignore
    let timeout = globalSetInterval(...args);
    return timeout[Symbol.toPrimitive]();
}
// @ts-ignore
global.WorkerGlobalScope = global;

Setting WebSocket to isomorphic-ws is good enough as a polyfill, but ideally, one could choose the implementation used between browser and ws. The crate's name is websys, so I do not know if this is a "good" idea, as it would go away from the websys part, then again, it's essential to have some sort even if only documentation as to how to set it up in a node environment.

setInterval on node returns a Timeout instance instead of a number, that then can be supplied to clearInterval. Either the conversion happens in the WebContext, or the value is no longer expected to be a number.

global.WorkerGlobalScope is set as otherwise WebContext panics, this "tricks" websocket-websys into thinking we're a webworker, even though we aren't. To properly support Node.JS, WebContext would likely need a new variant called Node or similar.

[thomaseizinger]

Thank you for opening this. We don't currently have or test support for NodeJS and I am not entirely sure we want to support it in a first-class way. How do other npm packages solve this problem? Do they detect that they are within NodeJS and use the appropriate APIs available? it is been a while that I did JS development but I'd see the responsibility on the NodeJS side to offer Web-compatible APIs. In other words, go with the polyfill method that you've already found to be working.

[indietyp]

In multi-context environments, packages usually use isomorphic-ws.

I have no problem adding that to the global scope, but the other hacks to make WebContext work worry me, as these changes to global functions have a non-negligible chance of impacting other running code.

In other words, I think what would need to happen is:

• WebContext gains the ability to detect NodeJS environments instead of returning None (leading to a panic)
• setInterval return values are no longer cast to a number and are taken as an opaque object.
• Changes to the documentation to signal that the polyfill is needed.

I'd see the responsibility on the NodeJS side to offer Web-compatible APIs

Usually, I'd agree with you here, and this would be something I'd expect as well (figuring this out surprised me), but this is the typical problem with JavaScript, the language (ECMAScript), and JavaScript's implementation. Still, the usage of NodeJS in the JS ecosystem warrants some special casing, especially if the changes required are relatively small.

[thomaseizinger]

setInterval return values are no longer cast to a number and are taken as an opaque object.

Yes, I think we've done something similar in the past actually. I can't find the piece of code now but I thought that we somehow managed to carry the interval handle as an opaque object. We might have to define our own bindings so that we can say it returns a JsValue instead of a number. This seems like a simple enough and isolated change that streamlines the usage of it in NodeJS environments. Would you be willing to submit a PR?

[thomaseizinger]

WebContext gains the ability to detect NodeJS environments instead of returning None (leading to a panic)

Perhaps even easier than the above, if we can reliably detect NodeJS, then we can just do the "right" thing here:

rust-libp2p/transports/websocket-websys/src/web_context.rs

Lines 34 to 56 in b6bb02b

	pub(crate) fn set_interval_with_callback_and_timeout_and_arguments(
	&self,
	handler: &::js_sys::Function,
	timeout: i32,
	arguments: &::js_sys::Array,
	) -> Result<i32, JsValue> {
	match self {
	WebContext::Window(w) => {
	w.set_interval_with_callback_and_timeout_and_arguments(handler, timeout, arguments)
	}
	WebContext::Worker(w) => {
	w.set_interval_with_callback_and_timeout_and_arguments(handler, timeout, arguments)
	}
	}
	}
	/// The `clearInterval()` method.
	pub(crate) fn clear_interval_with_handle(&self, handle: i32) {
	match self {
	WebContext::Window(w) => w.clear_interval_with_handle(handle),
	WebContext::Worker(w) => w.clear_interval_with_handle(handle),
	}
	}

[indietyp]

Thanks for the input. I have already created a PR, which should solve the problem using an approach similar to your suggested one!