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.

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

Added support for constructing from ArrayBuffer and added test that verifies support #42

Closed
wants to merge 1 commit into from
Closed

Added support for constructing from ArrayBuffer and added test that verifies support #42

wants to merge 1 commit into from

Conversation

robskillington
Copy link

No description provided.

@feross
Copy link
Owner

feross commented Sep 11, 2014

Thanks for the PR! I would like to accept this, but I'm very hesitant for the reasons mentioned here: #39 (comment)

length = +subject.length > 0 ? Math.floor(+subject.length) : 0
// Support ArrayBuffer subjects
if (subject.length === undefined && typeof subject.byteLength === 'number')
length = subject.byteLength
Copy link
Owner

Choose a reason for hiding this comment

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

It's actually not enough to just set the length correctly. subject itself should be wrapped in a Uint8Array so the data can actually be pulled out.

@robskillington
Copy link
Author

Thanks and apologies for not seeing other issues similar to this =]

@feross
Copy link
Owner

feross commented Sep 11, 2014

No problem. :) I'd open an issue with websocket-stream if you still think there's a bug here, though I expect you could just do new Buffer(new Uint8Array(arraybuffer)) in your own code.

@robskillington
Copy link
Author

So specifically mows which is an MQTT over WebSocket adapter is explicitly providing Uint8Array as the buffer type that websocket-stream should use: https://github.com/mcollina/mows/blob/master/buildWebsocketBrowser.js

This means it will new Uint8Array(incomingDataAsArrayBufferInstance) and then all the code therein breaks as websocket-stream is expecting a Nodejs Buffer API compatible buffer. Herein I updated to use just straight up Buffer using your fantastic work =] However I can't just provide type Buffer because this cannot build a Buffer from an ArrayBuffer. Hence here is my solution which I should ship back to mows now likely:

opts.type = BufferPatched;
//...
function BufferPatched(subject) {
    if (subject instanceof ArrayBuffer) {
        subject = new Uint8Array(subject);
    }
    return Buffer.apply(this, arguments);
}

util.inherits(BufferPatched, Buffer);

@feross
Copy link
Owner

feross commented Sep 11, 2014

This sounds like reasonable solution to me :)

@robskillington
Copy link
Author

Just thinking: would adding to the constructor of Buffer in this library add ArrayBuffer support for everyone?

    if (typeof ArrayBuffer === 'function' && subject instanceof ArrayBuffer) {
        subject = new Uint8Array(subject);
    }

@feross
Copy link
Owner

feross commented Sep 11, 2014

It would, and it's tempting to add it. But I'm worried that people would write code that depends on this behavior when it won't work in node.js. Maybe I should just send a PR to node core since this is an annoying API issue that keeps coming up.

@feross
Copy link
Owner

feross commented Jun 18, 2015

We can add support for this feature now that node.js#next has support! nodejs/node#2002

@feross
Copy link
Owner

feross commented Aug 5, 2015

ArrayBuffer can be passed into the constructor as of buffer 3.4.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