Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Abstract the ws package so we can change it (to use uws instead of ws for example) #67

Closed
kesne opened this issue Feb 5, 2017 · 24 comments

Comments

@kesne
Copy link

kesne commented Feb 5, 2017

I noticed you just recently switched to ws, and I was wondering if it might make sense to switch to uws, which has better performance.

A while ago I forked this module here to switch the websocket library to uws: https://github.com/kesne/subscriptions-transport-uws

Here's a quick post that features a quick overview of the performance benefits of using uws:
https://hackernoon.com/%C2%B5ws-as-your-next-websocket-library-d34209686357#.v5vyqggwq

@sibelius
Copy link

sibelius commented Feb 8, 2017

I think this should be another module instead

let's keep a package for ws and a package for uws

@DxCx
Copy link
Contributor

DxCx commented Feb 8, 2017

hi @kesne,
are the APIs the same?
from glance look it looks they are.
I am working on ws integration for graphql-server (apollographql/apollo-server#272)
and if they are i might just release the two variants togather...

@helfer
Copy link
Contributor

helfer commented Feb 10, 2017

It might be worth considering, but we should also keep in mind that ws has about 200x as many downloads on npm and many more production users, and is thus a somewhat safer choice for now.

@kesne Are you anticipating or seeing bottlenecks with ws?

@kesne
Copy link
Author

kesne commented Feb 10, 2017

uws has the same API as ws, with a few changes. Generally it's a drop-in replacement.

I haven't seen bottlenecks with ws, but we definitely saw memory improvements switching to uWS on an application with ~10,000 simultaneously connected users. I'll always pick a higher performance module if I can.

@helfer
Copy link
Contributor

helfer commented Feb 10, 2017

@kesne Makes sense. If you make a PR it's pretty likely to get merged 🙂

cc @Urigo

@DxCx
Copy link
Contributor

DxCx commented Feb 11, 2017

@kesne thanks!
I'm now planing to release graphql-server-uws once graphql-server-ws is getting released :)

@Urigo
Copy link
Contributor

Urigo commented Mar 11, 2017

forget my previous message..
does anyone here have a strong opinion about why not changing ws to uws?

@NeoPhi
Copy link
Contributor

NeoPhi commented Mar 11, 2017

Looks like uws doesn't have Hapi support which would be a show stopper for us:
https://github.com/uWebSockets/uWebSockets/issues/483

@Urigo
Copy link
Contributor

Urigo commented Mar 12, 2017

@NeoPhi good to know, I'm tracking it now.

@kesne in the meantime, maybe it will be a good idea to PR the repo's README to show that there is an option and direct them to your fork + maybe release it on npm?

@rclai
Copy link

rclai commented Mar 17, 2017

Isn't there a way to use optionalDependencies in the package.json to detect uws's existence and then use that instead? That's what socket.io does.

@Urigo
Copy link
Contributor

Urigo commented May 19, 2017

There are a lot of changes in the recent version, and it might be interesting to revisit this with the current master branch. @kesne or anyone else, would you be interested to open even a non-functioning PR? so we can start identify the needed changes and get this in?

@Urigo Urigo changed the title Use uws instead of ws? Abstract the ws package so we can change it (to use uws instead of ws for example) May 23, 2017
@vladshcherbin
Copy link

Would be great to have uws support, socket.io switched to using uws as a default one in 2.0.

@Urigo
Copy link
Contributor

Urigo commented Jun 20, 2017

hmmmm that's interesting.
Do you think we should change that before our upcoming 1.0 version?
can someone submit a PR?

@sergiodxa
Copy link

If uws it's a drop-in replacement for ws it's possible to use webpack aliases to replace them, just alias ws to uws. That way it's not necessary to change this module dependencies and you can change it if you want to.

@Urigo
Copy link
Contributor

Urigo commented Jun 20, 2017

@sergiodxa I'm not sure it is, would be great for someone to try and figure it out

@NeoPhi
Copy link
Contributor

NeoPhi commented Jun 21, 2017

Doesn't look like uws will support Hapi, given the comments closing out the issue referenced above. As such I think it would be best to make the transport library pluggable if more than ws is to be supported.

@marcodeltongo
Copy link

From what I understand that issue is related to the Node's core HTTP module experimental alternative which can be used by µWS but that miss many things like some core events; just enough for ws-only services.

The author stated in the new bindings repo:

The http server of uws is more of a gimmick and proof-of-concept. It is not currently developed any further and no, nothing of that works.

That said, I'm using µWS for websockets, using the http+https core Node modules as per documentation, in production it has great performance and low memory consumption on small AWS machines, both direct and behind ALB, from a React+Redux client (no GraphQL yet).

The differences from ws are here: https://github.com/uNetworking/bindings/tree/master/nodejs#deviations-from-ws

Apart Socket.io/Engine.io at least also Parse-Server, SocketCluster, DeepStream.io and Primus use it.

Hope it helps.

@rclai
Copy link

rclai commented Jul 19, 2017

Discord also uses it.

@maxpain
Copy link

maxpain commented Aug 26, 2017

+1

@tgdn
Copy link
Contributor

tgdn commented Aug 28, 2017

+1
uws could replace ws completely or have the option to use it instead of ws

@taion
Copy link

taion commented Jan 5, 2018

They seem compatible enough that this is all the handling Engine.IO needs: https://github.com/socketio/engine.io/blob/b1fa020675d905905b6f07d0a6b5acff775c4be5/lib/server.js#L105-L116

Given socketio/engine.io#550, perhaps uws is not a good default, but it seems easy to make it configurable.

@taion
Copy link

taion commented Jan 15, 2018

Seems like just dropping in uws doesn't work, BTW. Didn't find out why.

baconz pushed a commit to PhiloInc/subscriptions-transport-ws that referenced this issue Apr 24, 2018
@dalisoft
Copy link

Would be great to use uWebSockets.js instead ws

@wmertens
Copy link

wmertens commented Aug 6, 2019

what about https://github.com/HenningM/express-ws? That allows using the same port for websockets as for regular traffic. It's for express of course, but it would be nice to be able to use it somehow

@glasser glasser closed this as completed Mar 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests