-
Notifications
You must be signed in to change notification settings - Fork 114
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
Upgrade to ws v2.0.0+ to remove deprecation warnings #108
Comments
@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. Anyway, I'm open to options. |
We can make ws use
|
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. |
What? No, there are no API changes apart from |
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. |
@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. |
@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 This one is a bug in I'm not sure about this one instead but I guess it is because of a bug in 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? |
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. |
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 partially agree on this even if I think there are also some nice benefits.
We are well aware that Of course if the only reason to upgrade was the security fix, then there is no point in migrating. |
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.
Yeah, we'll do that when we bump the major here.
That was the most compelling reason, but keep the dependencies up-to-date is good :). |
Released as 4.0.0 |
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 ofbufferutil
. 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?
The text was updated successfully, but these errors were encountered: