Skip to content

support for binary websockets#6

Merged
max-mapper merged 3 commits intomax-mapper:masterfrom
juliangruber:binary
May 17, 2013
Merged

support for binary websockets#6
max-mapper merged 3 commits intomax-mapper:masterfrom
juliangruber:binary

Conversation

@juliangruber
Copy link
Contributor

The unopinionated API for using binary sockets (as in this PR) is:

// SERVER
// ...
var stream = wsStream(client, {
  binary: true,
  mask: false
});
// ...
// CLIENT
var stream = wsStream('ws://localhost:1337', { binaryType: 'arraybuffer' });

The opinionated and more elegant API (not coded yet) could be:

// SERVER
// var stream = wsStream(client, { binary : true });
// CLIENT
var stream = wsStream('ws://localhost:1337', { binary: true });

What do you think?

@max-mapper
Copy link
Owner

I think the version in this pull req is fine, I'd like this to stay as simple as possible.

another option is to make it so you can create your own WebSocket in the client or WebSocket-like object in node (require('ws').Client) and just do new wsStream(someExistingWebSocket)

I think this works in the server but not in the client?

also before I merge this can you double check that this isn't relevent https://twitter.com/maxogden/status/299248634996879360 also scroll to bottom of 15c4d4c

@juliangruber
Copy link
Contributor Author

The ws module requires an options argument to socket#send in order to send it as binary data.

ws works in the client too, but it has another API there, since it just exports the browser's native WebSocket implementation.

To send binary data from the client you do socket.send(<TypedArray>), whereas on the server you do socket.send(<Buffer>, { binary: true, make: false }).

To receive binary data on the client you do socket.binaryType = 'typedarray' whereas on the server I think you don't have to do that.

Hmm, need to investigate more. Having the same syntax on client and server would be great.

@max-mapper
Copy link
Owner

seems like the API on the server should be socket.send(<Buffer>) if you want binary and socket.send(<Buffer>.toString()) if you want ASCII

@juliangruber
Copy link
Contributor Author

@juliangruber
Copy link
Contributor Author

No you're right, it does a msg instanceof ArrayBuffer || msg instanceof Buffer check on the server and then sets the binary option automatically.

Isn't msg instanceof Buffer bad, since we have Buffer.isBuffer(buf)?

@max-mapper
Copy link
Owner

oh I was just saying thats the API I would write, not necessarily what I know the API that is implemented. also if you have thoughts on this TODO it could be useful here https://github.com/maxogden/level.js/blob/master/index.js#L70

@max-mapper
Copy link
Owner

cause if you try to write a typed array in node it would probably get sent as "[object Int8Array]"

@juliangruber
Copy link
Contributor Author

So let's make

stream.write('string');
stream.write(<TypedArray>/<Buffer>)

just work on both sides and on the client the default type of a received buffer is TypedArray?

@max-mapper
Copy link
Owner

sounds good! but I think it should be TypedArray/Buffer/ArrayBuffer (I think). also I dont really care about Buffer in the browser FWIW since they are super slow and just a hack for compatibility. I like https://github.com/chrisdickinson/bops better for solving that problem anyways

@juliangruber
Copy link
Contributor Author

What about

function test (obj) {
  return Buffer.isBuffer(obj) || /\[object (.+Array|Array.+)\]/.test(Object.prototype.toString.call(obj));
}
test(new Float32Array()) // true
test(new ArrayBuffer()) // true
test(new Buffer(3)) // true
test([]) // false

@juliangruber
Copy link
Contributor Author

Yeah, but people (or libraries!) will use buffers anyways...

@max-mapper
Copy link
Owner

yea I know :(

also +1 to that function, just put a comment in the code to explain what it does

@juliangruber
Copy link
Contributor Author

nah, that goes into npm anyway ;)

@max-mapper
Copy link
Owner

just checking - are you gonna update this pull req still?

@juliangruber
Copy link
Contributor Author

sec...:D

@juliangruber
Copy link
Contributor Author

should be good to merge now. I tested it with demo.js and the client received binary packets.

max-mapper added a commit that referenced this pull request May 17, 2013
support for binary websockets
@max-mapper max-mapper merged commit 3de7daf into max-mapper:master May 17, 2013
@max-mapper
Copy link
Owner

published as 0.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants