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

Streams should accept ArrayBuffers and/or any other browser objects equivalent to a Buffer #8

Closed
myndzi opened this issue Jun 16, 2015 · 15 comments

Comments

@myndzi
Copy link

myndzi commented Jun 16, 2015

See: http://stackoverflow.com/questions/30869460/node-js-browserify-error-on-parsing-tar-file/30878794#30878794

Once the responseType was set correctly, there was still an error with piping the response to the target stream since the response was an ArrayBuffer and not a "browserify Buffer". Would be a simple change that would save some people some grief.

@feross
Copy link
Member

feross commented Jun 16, 2015

Did you write this answer? There's actually an inaccuracy in your answer.

The Buffer shim in the browser accepts an ArrayBuffer

It actually doesn't. The Buffer constructor only accepts types that node.js does, and currently those are arrays and array-like objects (object containing a length property and integer indexes). ArrayBuffer is not array-like.

You should change new Buffer(arraybuffer) to new Buffer(new Uint8Array(arraybuffer)). Or use a module like typedarray-to-buffer to save a copy.

Note: This might change soon, though. See this issue: nodejs/node#106

@feross
Copy link
Member

feross commented Jun 16, 2015

Regarding changing the stream module to accept ArrayBuffers, that's not possible without significant changes to the internal code, because it assumes it's working with a buffer and has all the buffer methods available. We could convert to a buffer upfront, but should we do a copy or not? I think this decision is best left to the user.

But you're right, this user experience sucks.

The correct solution, IMO, is to make http.request, http.get, etc. return a buffer just like node does. That's the real inconsistency here that's causing confusion. No one should need to set the responseType property. Code written for node should just work in the browser.

I think this module was written before the browserify buffer was fast and reliable, so it might have made sense to design it this way. But this isn't true anymore, so we should update the module.

@feross
Copy link
Member

feross commented Jun 16, 2015

cc @jhiesey

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

I wrote the answer, and I also linked the source code directly to support my claim in that same statement...

See:
https://github.com/feross/buffer/blob/v3.2.2/index.js#L87-L88
to
https://github.com/feross/buffer/blob/v3.2.2/index.js#L121-L123

And from the readme: With browserify, simply require('buffer') or use the Buffer global and you will get this module.

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

You're correct, though, if I had thought about it I would have submitted this on the http module instead. I will do that.

Edit: I remember why I submitted here now; I assumed that http-browserify was returning a stream from this module. It looks like it instead implements its own streams1 api.

@feross
Copy link
Member

feross commented Jun 16, 2015

@myndzi I wrote the source code you linked to. I repeat: It doesn't accept ArrayBuffers.

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

I hadn't noticed that, but I'm confused as to why you would say that when it literally has and calls a function called 'fromTypedArray'? And also why the solution worked, in that case.

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

Ah, the confusion is in the phrasing. Setting responseType to 'arraybuffer' surely causes the response stream to emit instances of Uint8Array or similar. I was certain it worked because I actually was helping the original poster on IRC before I wrote up the answer.

Edit: OP confirms that the chunk being emitted in the data event is a Uint8Array already. It happens here:
https://github.com/substack/http-browserify/blob/master/lib/response.js#L107-L108

@feross
Copy link
Member

feross commented Jun 16, 2015

Typed Arrays are Uint8Array, Uint16Array, etc. All Typed Arrays point to a backing ArrayBuffer. ArrayBuffer is not a Typed Array. You can get the backing ArrayBuffer from any Typed Array by checking the buffer property. Example:

var typedarray = new Uint8Array(4)
var arraybuffer = typedarray.buffer

Yeah, it looks like http-browserify is actually emitting Uint8Arrays when you set the 'arraybuffer' responseType. Confusing. https://github.com/substack/http-browserify/blob/master/lib/response.js#L108

@feross
Copy link
Member

feross commented Jun 16, 2015

I've also seen ArrayBuffers described as Typed Arrays and Uint8Array, Uint16Array, etc. described as Typed Array Views.

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

I did understand the relationship between typed arrays and ArrayBuffer, but was confused by not knowing by heart what that meant as far as the browser APIs go, and the subtlety of the .buffer in the instanceof check. I updated the answer to be more correct.

Meanwhile, it would still be nice if the stream module understood "browser buffers", but it turns out that even if support was added, http-browserify wouldn't be able to take advantage of it to emit Buffer objects without a patch, so I may as well just ask for that directly. I'll open an issue over there, you can close this if you like.

@tejasshah93
Copy link

@feross the above mentioned question was from my side. I faced a scenario in which the code worked in node and didn't in browser. Now TBH, I myself didn't have much of the buffers knowledge. So it kept me wondering about transforming the response into actually what type of buffer. Then discussing with @myndzi on IRC narrowed it down.

So yes, from a third person view, what you mentioned to make http.request, http.get, etc. return a Buffer just like node does would be apt so that there's no need to set the responseType property and so on. @myndzi thanks for linking the issue over to http-browserify
@feross thanks for the overall clarity mentioned above. Appreciate that.

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

For clarity, I thought that this module was responsible for emitting the chunks, since http response objects are streams; turns out that's not the case.

Since the http-browserify module is the interface to XHR, it should probably be where the transformation from typed array to Buffer happens, though if this module provided an "in" for that, http-browserify could take advantage of that.

@feross
Copy link
Member

feross commented Jun 16, 2015

I'm not a collaborator on this repo, or I'd close this issue now.

@myndzi
Copy link
Author

myndzi commented Jun 16, 2015

Oh, right :)

@myndzi myndzi closed this as completed Jun 16, 2015
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

No branches or pull requests

3 participants