search

max-mapper / websocket-stream

websockets with the node stream API
BSD 2-Clause "Simplified" License
667 stars 114 forks source link

Update ws to version 3.x #123

Closed lpinca closed 7 years ago

lpinca commented 7 years ago

Here is a list of the breaking changes in ws@3: https://github.com/websockets/ws/releases/tag/3.0.0.

The most important is that the http.IncomingMessage object is no longer attached to the WebSocket object and is instead passed as the second argument to the 'connection' event. It seems that websocket-stream does not rely on the removed upgradeReq property so this should be a safe and easy update.

One thing to keep in mind is that stream.socket.upgradeReq is gone so this might be a breaking change.

mcollina commented 7 years ago

This is quite interesting, and in fact, huge 👍 to ship this through as it reduces memory consumption.

However it is a breaking change for everyone using this and relying on stream.socket.upgradeReq. I think we will need to bubble up that upgrade request to the end user in a similar way that ws do in here: https://github.com/maxogden/websocket-stream/blob/master/server.js#L14-L16.

Plus, we need to flip the default in the docs for https://github.com/maxogden/websocket-stream#optionspermessagedeflate.

cc @binarykitchen

lpinca commented 7 years ago

Yes, I checked few modules that use this as dependency and no one is using stream.socket.upgradeReq but ofc there might be some.