Eliminate native dependencies

[feross]

Because of the bufferutil and utf-8-validate dependencies in ws, many users see cryptic errors when installing ws or anything that depends on it.

Here's an example: "I can't build torrent-stream on my machine. It complains about MSB3428 (visual c++) when triggering node-gyp."

Here's another example, from my machine:

> bufferutil@1.1.0 install /usr/local/lib/node_modules/webtorrent/node_modules/torrent-discovery/node_modules/bittorrent-tracker/node_modules/ws/node_modules/bufferutil
> node-gyp rebuild

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

  CXX(target) Release/obj.target/bufferutil/src/bufferutil.o
  SOLINK_MODULE(target) Release/bufferutil.node

> utf-8-validate@1.1.0 install /usr/local/lib/node_modules/webtorrent/node_modules/torrent-discovery/node_modules/bittorrent-tracker/node_modules/ws/node_modules/utf-8-validate
> node-gyp rebuild

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

  CXX(target) Release/obj.target/validation/src/validation.o
  SOLINK_MODULE(target) Release/validation.node

Keep in mind for that last example of error spew, I actually have the OS X command line tools installed. Apparently, a compiler toolchain is still not enough. You actually need to install an IDE – Xcode. This is unreasonable.

It's true that these are optional dependencies... but npm's UX is really bad here. Error messages are displayed to the end user so they usually think something went wrong during installation. I keep getting asked about this.

The optional deps make it really hard to depend on ws in command line tools that normal users (i.e. not module authors) will be installing.

Can we discuss how we might eliminate these native dependencies?

cc @maxogden @mafintosh

[watdafox]

I absolutely haven't looked at the code, but there is probably 2 ways:

1. Write, rewrite or export in a lib (or inline) in pure JS to act similarly (probably lots of work)
2. Prebuild for each OS and ship it (heavy and inconvenient for updates)

[rom1504]

1. is already done there , that's what optional dependency usually means

( same thing here for utf-8-validate )

[3rd-Eden]

@feross This is something that npm should fix. The binary parts are optionalDependencies. If the UX is bad for the user it's because npm makes the ux bad. We as module authors cannot control the output for these modules.

Imho, npm should be pre-building binary addons and bundling them with dependencies during download. Having that said, there's already a pull request that allows building of prebuild binaries in the native modules but the problem here is shipping and installing.

[max-mapper]

@3rd-Eden what would be the downside of removing optionalDependencies from package.json? Users that need the perf boost can still install the optional dependencies manually. Users that don't need it (I would argue this is a larger group) won't see confusing output. Sounds like a win/win to me.

[3rd-Eden]

@maxogden The 2 binary addons are there for 2 different reasons:

1. the buffer utils includes performance enhancement for buffer operations and websocket masking.
2. the rfc specification requires UTF8 validation of messages to ensure that nothing bad or broken can be send over the connection, this utf-8 validation could not be done in JavaScript and needed to be written as binary addon. If this is removed the module will no longer be RFC compliant.

[max-mapper]

Since both of those cases are currently optional, why not ship it without
those and have instructions to install them separately? In my opinion
getting of the confusing ux of optionalDependencies is more important (and
easier to fix here than in npm).

On Tuesday, September 8, 2015, Arnout Kazemier notifications@github.com
wrote:

@maxogden https://github.com/maxogden The 2 binary addons are there for
2 different reasons:

1. the buffer utils includes performance enhancement for buffer
   operations and websocket masking.
2. the rfc specification requires UTF8 validation of messages to
   ensure that nothing bad or broken can be send over the connection, this
   utf-8 validation could not be done in JavaScript and needed to be written
   as binary addon. If this is removed the module will no longer be RFC
   compliant.

—
Reply to this email directly or view it on GitHub
#577 (comment).

Sent from my phone

[okdistribute]

+1, I'm recommending people not use optionalDependencies tag until they fix the UX.

Q: Is the module still RFC compliant if it isn't installed by default? also, @rom1504 just said that the utf-8 validation was done in JS with a link. Is that an acceptable replacement (and if so, why not include that in required deps?)

[mafintosh]

Another solution would be to create a new module that didn't have any optionalDependencies and was pure js and just have ws be a small wrapper around that that includes the native optionalDependencies.

[max-mapper]

@mafintosh that's a good idea. The main issue I have with the use of optionalDependencies in this module is that for the numerous modules I maintain that depend on this module I have absolutely no control over it. Would be nice if upstream authors could choose

[donaldpipowitch]

It's true that these are optional dependencies... but npm's UX is really bad here. Error messages are displayed to the end user so they usually think something went wrong during installation. I keep getting asked about this.

Crazy. They are optional?! I always wondered why everything works even if I see these errors.

[donaldpipowitch]

Imho, npm should be pre-building binary addons

Isn't that planned for npm@3? "built-in support for precompiled dependencies"

[max-mapper]

There are also various options ready to go today to handle prebuilt binaries that popular native addons like leveldown, sqlite3 use: https://www.npmjs.com/package/node-pre-gyp https://www.npmjs.com/package/prebuild

I think using either of the above modules would also help address this issue, but not solve it. I don't think waiting on npm is a good strategy when it can be fixed now.

I would personally like a way to use ws that never tries to compile anything.

[3rd-Eden]

An alternate solution would be to publish a new package that only has the native dependencies. A "ws-native" and a "ws". So we can have the best of both worlds and have authors decide on what they want to use

On Sep 9, 2015, at 2:11 AM, maxogden notifications@github.com wrote:

Since both of those cases are currently optional, why not ship it without
those and have instructions to install them separately? In my opinion
getting of the confusing ux of optionalDependencies is more important (and
easier to fix here than in npm).

On Tuesday, September 8, 2015, Arnout Kazemier notifications@github.com
wrote:

@maxogden https://github.com/maxogden The 2 binary addons are there for
2 different reasons:

1. the buffer utils includes performance enhancement for buffer
   operations and websocket masking.
2. the rfc specification requires UTF8 validation of messages to
   ensure that nothing bad or broken can be send over the connection, this
   utf-8 validation could not be done in JavaScript and needed to be written
   as binary addon. If this is removed the module will no longer be RFC
   compliant.

—
Reply to this email directly or view it on GitHub
#577 (comment).

Sent from my phone
—
Reply to this email directly or view it on GitHub.

[mafintosh]

@3rd-Eden that would work for our use case

[feross]

I like the idea of two packages: ws-native and ws.

And I like @mafintosh's suggestion for how to accomplish this. So we publish a new major version of ws that removes the optionalDependencies but adds a way for the user to pass in implementations of bufferutil and utf-8-validate (maybe these options remain undocumented). Then, we publish ws-native that depends on ws and the optionalDependencies and pass them into ws via the options.

That way we're not maintaining two parallel codebases.

[feross]

@3rd-Eden What do you think of my last suggestion? Would you accept a PR with those changes?

[feross]

@3rd-Eden Ping! I have some free time now. Would you accept a PR that implements #577 (comment) ?

[feross]

Oooh, nice – I see that you just committed this change! Excellent, I will close this issue now.

[feross]

For posterity: 49b1109