HenningM / express-ws

WebSocket endpoints for express applications
BSD 2-Clause "Simplified" License
878 stars 142 forks source link

Use the latest `ws`? #90

Closed danfuzz closed 6 years ago

danfuzz commented 6 years ago

It looks like ws is two major revs ahead of what express-ws lists in its dependencies. Perhaps worth moving to it?

AuspeXeu commented 6 years ago

Did you test it with the latest version of ws @danfuzz ?

danfuzz commented 6 years ago

@AuspeXeu: I didn't. From the ws changelog https://github.com/websockets/ws/releases/tag/4.0.0, it looked like there were enough breaking changes that it wouldn't be trivial for someone like me who isn't familiar with the code (of any of express-ws, express, or ws) to make headway and be confident of a good result.

I just happened to notice the datedness of the dependency when I was tracking down an issue in my own code (which I thought might have been coming from express-ws but in the end wasn't). Rather than silently noting it and moving on, I figured I could at least raise the issue here.

Cheers!

AuspeXeu commented 6 years ago

That is also how I ended up here haha Edit: Looking at the files in https://github.com/HenningM/express-ws/tree/master/src it does not look like there is too much logic to it though...

danfuzz commented 6 years ago

Huh, perhaps the dependency on ws from this module would be more profitably expressed under peerDependencies instead of plain old dependencies.

AuspeXeu commented 6 years ago

@danfuzz I think I am switching to the bare ws module using this example.

https://github.com/websockets/ws#expressjs-example

theflyingape commented 6 years ago

express-ws fails an npm audit referring to this DoS vulnerability: https://nodesecurity.io/advisories/550

pixelspark commented 6 years ago

Ran into the audit warning as well. Any progress?

danfuzz commented 6 years ago

As with @AuspeXeu I've now just dropped my use of this module. In case it helps, here's the commit in my project that made the change: https://github.com/danfuzz/bayou/pull/466/commits/4ed5569049c138e0e26fb55b1c069e2be8c0a1a6

pixelspark commented 6 years ago

Ah that is easier than I thought. Thanks for the pointer, will use this (perhaps a notice in express-ws's README would be a good idea?)

danfuzz commented 6 years ago

BTW, one thing to watch out for is that my change makes it so that express isn't involved at all in websocket requests. This means that any route intercepters / middleware you might have installed — notably logging — won't get invoked.

theflyingape commented 6 years ago

Same here, still using Express for REST APIs, but replaced express-ws using only ws 'upgrade' endpoints to handle the socket connections, without adding more URL paths (because this runs behind Apache proxy), sigh.