Upgrade to ws v2.0.0+ to remove deprecation warnings

[simonmcl]

Hi,

I'm using a node_module that depends on one of your versions. The version of ws that your latest release is using, is using an outdated version of bufferutil. On Node.js v6.0.0+ this version of bufferutil is producing a large stack trace in the console with deprecation warnings.

ws v2.0.0 bumped up the support of bufferutil to the minimum version needed to fix the issue. Would you be able to incorporate this?

[mcollina]

@simonmcl #106. Please review.

The problem of ws v2.0.0 is that it requires node v4.5.x minimum. I do not know if @mafintosh or @maxogden are happy with that. I am not, as I think it's unneeded disruption of the community.
I have some modules that depends on this one, and the chain of bumping major version numbers is costly.

Anyway, I'm open to options.

[lpinca]

We can make ws use safe-buffer and support all node v4 versions but that's it. Node.js 0.10 and 0.12 reached EOL since quite some time now.

ws@1 works but keeping using it is not a good idea imho. A lot of bugs have been fixed in version 2.

[mcollina]

ws v2 has a very different API, and it embraces ES6 fully.

Most of those bug fixes did not require a change of the external API, nor a change in the supported version of Node.

I'm open if anyone wants to help, as it seems I am the only one maintaining this.

[lpinca]

ws v2 has a very different API, and it embraces ES6 fully.

What? No, there are no API changes apart from ws.{ping,pong}() and ws.stream() which are not used here and only ES6 features supported in Node.js 4 are used ofc. This allowed us to cut memory usage in half.

[lpinca]

Some things like websockets/ws#871, websockets/ws#651, websockets/ws#885 required a major version bump.

Backporting all the possible fixes (some can't be backported) to 1.x is an option but again I see no reason to do that since node 0.10 and 0.12 are no longer supported.

Anyway the decision on whether to upgrade or not is ultimately up to the maintainers of this lib, I'm only expressing my opinion as one of the maintainers of ws.

[mcollina]

@lpinca if what you said was true, it would have only required 1 minute to prepare a PR. Instead, all tests went red, and I spent roughly 1 hour to figure out why in all cases. Most of those changes were done to adopt ES6 classes and lingo, rather than actual feature changes.

Anyway, having safe-buffer in ws and supporting node 4.0.0 would help us in migrating.

[lpinca]

@mcollina from your PR https://github.com/maxogden/websocket-stream/pull/106/files?w=1 the only really required changes I see are due to the inheritance with ES6 classes. The use of ES6 classes is documented as breaking change in ws@2.

This one is a bug in ws@2. It should be fixed and a regression test added. I didn't even know about this. No one reported this regression.

I'm not sure about this one instead but I guess it is because of a bug in ws@1.

I still fail to see why you think that API is "very different". As I see it you had to fix inheritance, and work around a regression bug. Where are the API changes?

[mcollina]

I think we have a different idea of API changes then. Those two side-effects were not simple to debug, if you consider the only reason for upgrading was a security fix.

I also think that changing the inheritance model forces people to change existing, working code for no benefit. This is my personal taste, and I do not like being forced to do work. I tend to dislike ES6 classes very much for this reason, so we might happily disagree on this issue.

I still think we should upgrade websocket-stream at some point in the near future and get rid of 0.10 and 0.12. This does not change the fact that we still have a lot of users depending on ws v1.

[lpinca]

I think we have a different idea of API changes then.

Yeah probably. If a fix breaks existing code because the code was relying on faulty behavior it's a breaking change but it's not an API change imho. The interface is exactly the same.

I also think that changing the inheritance model forces people to change existing, working code for no benefit.

I partially agree on this even if I think there are also some nice benefits.

This does not change the fact that we still have a lot of users depending on ws v1.

We are well aware that ws@1 is still widely used and that's why we did backport the most important fixes but I recommend migrating as soon as possible. Take a look as ws@1 code if you ever find some time ;)

Of course if the only reason to upgrade was the security fix, then there is no point in migrating.

[mcollina]

Yeah probably. If a fix breaks existing code because the code was relying on faulty behavior it's a breaking change but it's not an API change imho. The interface is exactly the same.

Yup! That code worked fine in the browser too, so I fear the problem is in ws@2 and ws@1 was the correct behavior.

We are well aware that ws@1 is still widely used and that's why we did backport the most important fixes but I recommend migrating as soon as possible. Take a look as ws@1 code if you ever find some time ;)

Yeah, we'll do that when we bump the major here.

Of course if the only reason to upgrade was the security fix, then there is no point in migrating.

That was the most compelling reason, but keep the dependencies up-to-date is good :).

[lpinca]

It seems that you use two different functions, one for ws and one for the browser so I'm not sure.

If you call ws.send() after calling destroy() which in turn calls ws.close() then the error is expected and ws@2 is correct. I'll take a look when I can.

[mcollina]

Released as 4.0.0