pass Array Buffer to constructor

[calvinmetcalf]

No description provided.

[feross]

This isn't actually checking the right buffer, b2.

[calvinmetcalf]

do'h all I checked was that it failed without this patch

[feross]

Hi, thanks for the PR! I don't think this is a good idea (aside from the issues I mention inline) because it would make this module diverge from the node.js buffer API.

An ArrayBuffer is not array-like: it lacks a length property indexes can't be accessed directly via buf[0], etc. Thus, new Buffer(new Uint8Array([0, 1, 2, 3]).buffer) fails in both node.js and this module.

If you have an ArrayBuffer, just wrap it in a Uint8Array before passing it to the constructor and you'll be good.

[calvinmetcalf]

so the impetus for this was a bug using level-js (Level/level-js#34) caused by array buffers being turned into zero length Uint8Array objects, there is some code that made me think that you intended to support array buffers, but if you don't would you accept a pull to throw or print a warning if array buffers are given to the constructor (yes strictly speaking a divergence from node, but your more likely to get a typed array in browserified contexts)

[feross]

The code you linked is there to support TypedArrayViews like Uint8Array, etc. which have a byteLength property as well. The set function wouldn't work with an arraybuffer. :)

If level-js claims to support ArrayBuffers here (I don't know if it does), then this is a bug in how it's using this module. It should check if it's an ArrayBuffer and wrap it in a Uint8Array.

[calvinmetcalf]

right forgot the typed arrays had both

On Fri, Aug 1, 2014 at 8:16 PM, Feross Aboukhadijeh <
[email protected]> wrote:

The code https://github.com/feross/buffer/blob/master/index.js#L96 you
linked is there to support TypedArrayViews like Uint8Array, etc. which have
a byteLength property as well. The set function wouldn't work with an
arraybuffer. :)

If level-js claims to support ArrayBuffers here (I don't know if it does),
then this is a bug in how it's using this module. It should check if it's
an ArrayBuffer and wrap it in a Uint8Array.

—
Reply to this email directly or view it on GitHub
#39 (comment).

-Calvin W. Metcalf

[feross]

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

[feross]

ArrayBuffer can be passed into the constructor as of buffer 3.4.0.

[calvinmetcalf]

vindicated! thanks @feross

[feross]

@calvinmetcalf Yeah :) I agreed it's a great idea but want to keep this module exactly in sync with node. I argued to support this - happy it finally landed!!