decodePacket now accepts both Buffer and ArrayBuffer as data

[Rainforce15]

Potential fix for Issue #60.

[Rainforce15]

This is potentially not in line with how you planned the decode operation to work (only ever accepting Buffers) in a node-like environment. If it shouldn't be fixed in this part of the code, it might be a bug in browser detection/how nw.js manages its browser-websockets. Please give feedback.

[darrachequesne]

@Rainforce15 could you please add some test, to prevent any future regression?

[Rainforce15]

added Tests and fixed code to run properly from 0.10 onwards.
There seems to be a problem with Saucelabs and their iPhone browser 7.1 support, though (I checked, I don't seem to be able to select them on the site for new Tests, either)
They just return "no tests ran"

[darrachequesne]

how about using arrayBufferToBuffer method here?

if (binaryType === 'arraybuffer') {
  data = arrayBufferToBuffer(data);
}

[darrachequesne]

You should be able to remove the done() here, since there is no callback

[Rainforce15]

Thank you for taking the time to review my code.
You are right, I didn't check for the existence of a helper function.
Tests work with arrayBufferToBuffer.
Done() cannot be removed, at least when I try to run the tests.
Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

[darrachequesne]

I guess you also have to remove the done callback here: it('should decode an ArrayBuffer/Buffer with undeclared type as binary', function(done) {

[darrachequesne]

Please correct me if I'm wrong, but aren't those cases already handled in the existing tests? To me, the missing test might be something like:

  it('should encode/decode an ArrayBuffer as binary (with undefined binaryType)', function(done) {
    var buffer = new ArrayBuffer(4);
    var dataview = new DataView(buffer);
    for (var i = 0; i < buffer.byteLength ; i++) dataview.setInt8(i, i);

    encode({ type: 'message', data: buffer }, true, function(encoded) {
      var decoded = decode(encoded);
      expect(decoded).to.eql({ type: 'message', data: new Buffer([0,1,2,3]) });
      expect(new Uint8Array(decoded.data)).to.eql(new Uint8Array(buffer));
      done();
    });
  });

[Rainforce15]

The existing tests mostly test that running encode->decode works in succession - without ever checking the intermediate result of encode() or working of the functions on their own.(or what kind of data they create on different platforms) This might work for most cases, but since decode() is a publically available function, it can be called from anything and could use some more error proofing than just giving them controlled input from encode of the same platform, depending on design/intention, in my opinion.
In fact, you could create intermediate data that is against the design and would never notice with this test as long as you make the same "mistake" in both the output of encode() as well as the input of decode() (which apparently happens in nw.js and others). My experience with testing is very limited though, as is my understanding of the overal intention of how to use the code and under which circumstances.

[darrachequesne]

My concern is that some of the tests you added may already exist elsewhere in the file.

I agree though that the output of both method encode and decode could be tested, but that seems out of the scope of that particular PR, doesn't it?

[Rainforce15]

It tests that Buffer and ArrayBuffer both work as in- and output of decode(), which is what this fix is about (the intent that it fixes cross-platform issues in Socket.io and Node.js/nw.js/Electron when they use encode()/decode() because they can be of both types)
Therefore, I think the test is okay as it is creating a Buffer and ArrayBuffer, and testing it once with arrayBuffer as binaryType and another time with no binaryType (might as well be "message" though, afaik).

If this is beyond the intended scope of the function, please reject this PR.

[darrachequesne]

@Rainforce15 thanks a lot, merged as 5aecaa9