search

max-mapper / websocket-stream

websockets with the node stream API
BSD 2-Clause "Simplified" License
668 stars 114 forks source link

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

Closed binarykitchen closed 8 years ago

mcollina commented 8 years ago

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 commented 8 years ago

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

binarykitchen commented 8 years ago

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

binarykitchen commented 8 years ago

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

mcollina commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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: https://github.com/maxogden/websocket-stream/pull/83#discussion_r50798418 -- 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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

deanlandolt commented 8 years ago

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 commented 8 years ago

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

binarykitchen commented 8 years ago

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!