Skip to content

Commit

Permalink
[major] No longer use the binary addons as optional dependency, nuked…
Browse files Browse the repository at this point in the history 
… completely
  • Loading branch information
@3rd-Eden
3rd-Eden committed Dec 21, 2015
1 parent 4423b6e commit 49b1109
Showing 1 changed file with 3 additions and 5 deletions.
8 changes: 3 additions & 5 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,15 @@
"options": ">=0.0.5",
"ultron": "1.0.x"
},
"optionalDependencies": {
"bufferutil": "1.2.x",
"utf-8-validate": "1.2.x"
},
"devDependencies": {
"ansi": "0.3.x",
"benchmark": "0.3.x",
"bufferutil": "1.2.x",
"expect.js": "0.3.x",
"mocha": "2.2.x",
"should": "4.3.x",
"tinycolor": "0.0.x"
"tinycolor": "0.0.x",
"utf-8-validate": "1.2.x"
},
"browser": "./lib/browser.js",
"component": {
Expand Down

10 comments on commit 49b1109

@SEAPUNK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this mess up UTF-8 validation, though? I'm guessing that since utf-8-validate is now a dev dependency, the Autobahn tests would pass normally, but when you actually install the library, UTF-8 validation wouldn't work as expected, since the fallback returns true unconditionally...

@3rd-Eden
Copy link
Member Author

@3rd-Eden 3rd-Eden commented on 49b1109 Dec 22, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SEAPUNK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the module be fixed (at least in theory), or is there another module that can replace it?

@SEAPUNK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything, websockets/utf-8-validate#23 seems to be a decent fallback.

@3rd-Eden
Copy link
Member Author

@3rd-Eden 3rd-Eden commented on 49b1109 Dec 22, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SEAPUNK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simply because JavaScript as a language it self cannot do the UTF8 validation as required by the spec
But ws isn't a library for anything that uses JavaScript, it's a library for Node.js (or at least your repo description says so)

While it can't do UTF8 validation, Node.js at least knows when to replace invalid UTF8 characters with U+FFFD (REPLACEMENT CHARACTER). The Buffer.compare(new Buffer(buff.toString(),'utf8') , buff) === 0 workaround is an indirect validation, of sorts.

I ran the Autobahn fuzzingclient suite for the 6.* tests, and it passed them all with the proposed workaround, whereas the return true function fails the tests where Autobahn sends invalid UTF-8 data.

But if we are realistic here, everybody should be validating the user input they receive and therefor already validating the data them selfs

I would consider that unexpected/inconsistent behavior, though. If the RFC says the implementation has to drop the connection, then I'd assume you have to drop the connection. If you had, say, a chat application that basically broadcasted whatever message someone sent to a chat room, I'd honestly prefer it to only broadcast message that has U+FFFDs in them when explicitly requested in valid UTF-8 form.

@SEAPUNK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: A native UTF8 validator is in progress in Node: nodejs/node#1319

@sontek
Copy link

@sontek sontek commented on 49b1109 Feb 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke my setup because I only have python 3 and node-gyp isn't compatible with it. Any way we can get this reverted?

I ended up added bufferutil as an optionalDependency in my own packages.json which seems to override it

@sontek
Copy link

@sontek sontek commented on 49b1109 Feb 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is how to fallback with it:

https://github.com/websockets/bufferutil/blob/master/fallback.js

@ozyman42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the installation still fails when these fail to install as optional dependencies, doesn't that mean we have found a bug in npm? npm's own site says that installation failures of individual optional dependencies should not cause the entire npm install operation to fail.

Please sign in to comment.