Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 26, 2016
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ module.exports = WebSocketStream

function WebSocketStream(target, protocols, options) {
var stream, socket
var socketWrite = process.title === 'browser' ? socketWriteBrowser : socketWriteNode

var isBrowser = process.title === 'browser'
var isNative = !!global.WebSocket
var socketWrite = isBrowser ? socketWriteBrowser : socketWriteNode
var proxy = through.obj(socketWrite, socketEnd)

if (protocols && !Array.isArray(protocols) && 'object' === typeof protocols) {
Expand All @@ -19,6 +22,7 @@ function WebSocketStream(target, protocols, options) {

// browser only: sets the maximum socket buffer size before throttling
var bufferSize = options.browserBufferSize || 1024 * 512

// browser only: how long to wait when throttling
var bufferTimeout = options.browserBufferTimeout || 1000

Expand All @@ -27,7 +31,14 @@ function WebSocketStream(target, protocols, options) {
socket = target
// otherwise make a new one
} else {
socket = new WS(target, protocols, options)
// special constructor treatment for native websockets in browsers, see
// https://github.com/maxogden/websocket-stream/issues/82
if (isNative && isBrowser && !protocols) {
socket = new WS(target)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

} else {
socket = new WS(target, protocols, options)
}

socket.binaryType = 'arraybuffer'
}

Expand Down