Omit protocols for native WebSockets in browsers only. Fixes TypeErrors, see issue 82

[binarykitchen]

No description provided.

[mcollina]

Just to clarify, is this a Safari only issue? test worked fine in Chrome.
In case, would you mind adding that in the comment?

[jnordberg]

With this logic I can't see any way to pass protocols without also passing options as the 3rd argument. Wouldn't that cause the same error this PR is trying to fix for anyone wanting to use specific subprotocols?

[binarykitchen]

according to the w3.org, there is no options parameter for native websockets, see https://www.w3.org/TR/2011/WD-websockets-20110419/#the-websocket-interface

[jnordberg]

Yes, but I'm talking about the optional protocols argument which is in the spec.

[binarykitchen]

right. have a look at the if clause if (isNative && isBrowser && !protocols) {

it skips when a protocol exist and executes the code in the else clause which passes on the optional protocols argument

[deanlandolt]

Wouldn't this fail in the case where you need to pass protocols to a native websockets impl? From some testing in safari it looks like any call w/ arguments.length > 2 will throw a TypeError.

ISTM we could just drop the !protocols check from the first case -- something like this:

if (isNative && isBrowser) {
  socket = new WS(target, protocols)
} else {

WFM at least.

[mcollina]

I agree that we need to pass protocols from the browsers. This is used by MQTT.js (as an example).

[jnordberg]

Maybe a simpler solution to this would be to let ws-fallback.js drop the options argument?

[binarykitchen]

@mcollina no, it also happens on chrome under mac osx, not just safari

[binarykitchen]

@jnordberg i do not follow you - feel free to add this with or after my commit

[mcollina]

@binarykitchen I've tested with latest Chrome and latest Safari and I do not see the issue in #82.

Which version of Safari/Chrome are you having issues with?

[binarykitchen]

@mcollina Chrome v31.0.1650.63 on Mac OS X 10.11.2 after running for two days without any restarts.

Haven't tested this issue on Safari.

The bug is difficult to reproduce I must admit. If you want to be 100% sure, maybe raise this on the Chromium Bug Tracker and question the native WebSocket implementation there?

[deanlandolt]

@binarykitchen FWIW this issue is easy for me to repro in Safari on OSX 10.10.4 (though I've never hit it in chrome). See: #83 (comment) -- this should let us pass protocols in all cases.

Another alternative is just wrapping in a try and falling back to the 2-arity call, w/ perhaps another try falling back to the 1-arity call (sans protocols). Kinda sloppy, but ultimately still leaves us in a predictable state.

[mcollina]

My problem here is that I cannot reproduce, and it does not seem an error that could pops up after 2 days of activity.

@deanlandolt if you can reproduce consistently, would you mind checking if passing protocols in all cases is good, i.e. if the problem is only in passing options?

If that's the case, I presume that is the way to go.

@mafintosh @maxogden what do you think?

[deanlandolt]

I verified (on OSX 10.10.4 in Safari 8.0.7) that passing protocols is safe when undefined, null, or []. I can't say whether it would throw on specific values of protocols, but presumably we'd want it to fail loudly in these cases anyway. I also verified that passing a third option (even undefined) always throws.

[mcollina]

@binarykitchen would you mind updating your PR and removing the protocols check and passing it back to the constructor?

[deanlandolt]

I wasn't sure what the preferred workflow was for this case, so I just sent a PR on top of this commit's PR to @binarykitchen branch: https://github.com/binarykitchen/websocket-stream/pull/1

[mcollina]

you can also send another PR here, I'll get that merged and released asap.

[binarykitchen]

sorry for my late response. was away and then github was down heh.

thanks @deanlandolt for your PR. had a quick look, all looks. thanks guys!