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

Update ws to version 3.x #123

Merged
merged 3 commits into from
May 22, 2017
Merged

Update ws to version 3.x #123

merged 3 commits into from
May 22, 2017

Conversation

lpinca
Copy link
Contributor

@lpinca lpinca commented May 21, 2017

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
Copy link
Collaborator

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
Copy link
Contributor Author

lpinca commented May 21, 2017

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

@mcollina mcollina merged commit 42a155a into max-mapper:master May 22, 2017
@lpinca lpinca deleted the update/ws branch May 22, 2017 07:08
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.

2 participants