max-mapper / websocket-stream

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

TypeError: Type error #82

Closed binarykitchen closed 8 years ago

binarykitchen commented 8 years ago

This error is thrown at this line

socket = new WS(target, protocols, options)

(https://github.com/maxogden/websocket-stream/blob/master/stream.js#L30)

parameters were

target: "wss://www.videomail.io?x-videomail-site-name=videomail.io”
protocols: undefined
options: Object
    __proto__: Object

User-Agent is Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Any ideas?

binarykitchen commented 8 years ago

FYI this is my app code calling your websocket function https://github.com/binarykitchen/videomail-client/blob/develop/src/wrappers/visuals/recorder.js#L379

binarykitchen commented 8 years ago

I guess it is related to https://github.com/socketio/engine.io-client/pull/375

If protocols is undefined, do not pass it onto the native constructor!

binarykitchen commented 8 years ago

@mcollina

mcollina commented 8 years ago

From https://www.w3.org/TR/2011/WD-websockets-20110419/#the-websocket-interface:

The WebSocket(url, protocols) constructor takes one or two arguments. The first argument, url, specifies the URL to which to connect. The second, protocols, if present, is either a string or an array of strings. If it is a string, it is equivalent to an array consisting of just that string; if it is omitted, it is equivalent to the empty array. Each string in the array is a subprotocol name.

I think that protocols variable needs to be omitted if protocols is empty, but only in Browsers.

@binarykitchen would you mind sending a PR?

binarykitchen commented 8 years ago

Here you go https://github.com/maxogden/websocket-stream/pull/83

Please review and be aware that https://www.w3.org/TR/2011/WD-websockets-20110419/#the-websocket-interface also does not mention of options, hence I omitted them too, but only for native WebSocket implementations.

All tested on my side, everything passes.

binarykitchen commented 8 years ago

thanks for the comments inside the PR. have updated my PR slightly. please re-check.

mcollina commented 8 years ago

Probably fixed.

kursatarslan commented 4 years ago

I have same issue. image