search

max-mapper / websocket-stream

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

Bumped ws to v2.0.0. #106

Closed mcollina closed 7 years ago

mcollina commented 7 years ago

Fixes #105.

This is a semver-major change, as it will work only on Node v4+.

antoniobusrod commented 7 years ago

Don't you prefer to stay at next https://github.com/websockets/ws/releases/tag/1.1.2 ?

mcollina commented 7 years ago

At some point we will have to migrate to v2.

sublimator commented 7 years ago

https://github.com/websockets/ws/releases/tag/2.0.0

Dropped support for Node.js < 4.5.0.

Man, AWS lambda is stuck on 4.3.2, they need to get their act together.

simonmcl commented 7 years ago

Linked here from @mcollina

My 2 cents:

I understand the issue and the effort involved. While my understanding of this module is limited as I'm using one which depends on this, there is a very long stack trace appearing in the console coming from v8 directly, when using Node v6+, to say that the version of bufferutils being used, is using deprecated methods (bufferutils is being used by ws).

We faced a lot of errors and crashes in Node v7 when trying to use the module, and ultimately couldn't get it to work. I realise the drop in support is not ideal, but it may become necessary very soon.

mcollina commented 7 years ago

@simonmcl we are not using bufferutils anywhere in this module. websocket-stream works without problem on v7 on my machine. What errors are you seeing on Node v7? Can you open specific issues on those, or maybe contribute some unit tests so that we can improve things?

simonmcl commented 7 years ago

@mcollina bufferutils is a dependency of ws, which is a dependency of your module. We didn't have time to debug the v7 issue on our current project, so we just stayed with v6.9.5, we will look to upgrade another time when we have the bandwidth, just guessing it might have been related due to the deprecation warnings.

But when trying to use https://www.npmjs.com/package/ibmiotf , on node 6.9.5 a large stacktrace appears warning of deprecations for bufferutil. They are using mqtt, which is using websocket-stream.

I planned to ask them to update their dependencies, but discovered that there was no version of websocket-stream which had the appropriate version of ws to have the appropriate version of bufferutils

... anyone else's head hurting or just me?

mcollina commented 7 years ago

@simonmcl bufferutil is not a dependency of ws v1: https://github.com/websockets/ws/blob/v1.x/package.json#L24.

So, something else should be going on in here. I think you should open an issue on ibmiotf, and we can work with them to solve the issue. They probably have more bandwidth than us anyway. Feel free to tag me in that issue.

simonmcl commented 7 years ago

@mcollina ah its a devDependency, I just searched for bufferutil, missed what brace it was in. Ok cool, i'll check this issue again and follow up with them then. Thanks for the help

simonmcl commented 7 years ago

@mcollina had another few minutes to look at this. Turns out the versions being used by that IOT module, is using a version of ws where bufferutil is listed as an optional dependency. It was moved to a dev dependency later on. Running npm install --no-optional solves the issue for now.

Logged an issue with the IOT module to update their versions.

Thanks for the help debugging this!

sublimator commented 7 years ago

U using aws IoT too?

simonmcl commented 7 years ago

@sublimator no IBM's IOT platform on Bluemix

sublimator commented 7 years ago

I saw aws/azure use mqtt 1.x (though not sure about the latter)

sublimator commented 7 years ago

ws 2 does not work on aws ... they are still on node 4.3.2 ...

simonmcl commented 7 years ago

@sublimator 4.3.2? jesus thats horrific, my thoughts and prayers are with you at this difficult time lol

sublimator commented 7 years ago

Hahaha

lpinca commented 7 years ago

FWIW aws now (Mar 22nd, 2017) supports and recommends Node.js v6.10

sublimator commented 7 years ago

Yeah, aws 6.10!

mcollina commented 7 years ago

Still, a lot of people uses older version of v4. Can we get ws to work on v4.0.0? Lamda's node 4.3.2 is still there for a lot of people.

lpinca commented 7 years ago

Node.js v4.5 is only required due to the use of safe Buffer APIs only available is 4.5+ but we can use https://www.npmjs.com/package/safe-buffer in ws to support all v4 versions.

mcollina commented 7 years ago

@lpinca is the perMessageDeflate option understood also in the browser, or is it node-specific?

mcollina commented 7 years ago

@lpinca forget about that question, we are not passing the options through.

lpinca commented 7 years ago

The are no options in the browser interface. It depends on the browser. It is automatically negotiated when supported (if the server supports it ofc).