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

[DOC] Buffer.from / new Buffer accept Uint8Array (missing) #14118

Closed
reviewher opened this issue Jul 7, 2017 · 6 comments
Closed

[DOC] Buffer.from / new Buffer accept Uint8Array (missing) #14118

reviewher opened this issue Jul 7, 2017 · 6 comments
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@reviewher
Copy link

Both can accept a Uint8Array. Buffer.from explicitly checks for Uint8Array

It looks like a documentation omission. If so, I can send a PR.

@Trott
Copy link
Member

Trott commented Jul 7, 2017

@nodejs/documentation

@Trott Trott added doc Issues and PRs related to the documentations. buffer Issues and PRs related to the buffer subsystem. labels Jul 7, 2017
@benjamingr
Copy link
Member

@reviewher the docs already specify it takes a TypedArray - the fact that the implementation does a shortcut should not be an issue.

Was the unclear part the fact it's not obvious that a UInt8Array is a TypedArray?

@reviewher
Copy link
Author

@benjamingr The documentation lists 4 signatures for Buffer.from:

  • Buffer.from(array) -- Array of bytes
  • Buffer.from(arrayBuffer[, byteOffset[, length]]) -- ArrayBuffer or the .buffer property of a TypedArray.
  • Buffer.from(buffer) -- Buffer
  • Buffer.from(string[, encoding]) -- string

None of those cases cover an actual Uint8Array object! We are not talking about "the .buffer property" but rather the actual Uint8Array object. An example would be useful:

> var x = new Uint8Array([1,2,3,4,5])
> x
Uint8Array [ 1, 2, 3, 4, 5 ]

> Buffer.from(x.buffer) // <-- this is from the docs
<Buffer 01 02 03 04 05>
> x.buffer instanceof ArrayBuffer // <-- x.buffer is an ArrayBuffer, not a typed array
true
> x.buffer instanceof Uint8Array // <-- x.buffer is definitely not a Uint8Array
false

> Buffer.from(x) // <-- this is the case that works and is not covered in the docs
<Buffer 01 02 03 04 05>
> Buffer.isBuffer(x) // <-- x is definitely not a buffer
false
> x instanceof ArrayBuffer // <-- x is definitely not an ArrayBuffer
false
> x instanceof Uint8Array // <-- x is definitely a Uint8Array
true

To contrast with other areas of the documentation:

@benjamingr it's also possible you took a cursory glance at https://nodejs.org/dist/latest-v8.x/docs/api/buffer.html#buffer_buffers_and_typedarray and assumed it was covered, but alas it is not. The ending of that section does discuss typed arrays, but it is merely contrasting the signatures of Buffer.from and TypedArray.from.

@Trott to be consistent with the other areas of the documentation, assuming that Buffer.from and the new Buffer constructor were intended to take Uint8Array, the simplest fix would add a few words the Buffer cases in new Buffer and Buffer.from in doc/api/buffer.md :

-* `buffer` {Buffer} An existing `Buffer` to copy data from
+* `buffer` {Buffer|Uint8Array} An existing `Buffer` or [`Uint8Array`] to copy data from

@apapirovski apapirovski added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. good first issue Issues that are suitable for first-time contributors. labels Apr 11, 2018
@apapirovski
Copy link
Member

@nodejs/documentation This has been around for a while and looks like a valid bug report for the docs.

@reviewher
Copy link
Author

This is a one-line change to /doc/api/buffer.md, included in my comment in this issue. I'll gladly send a PR, but @benjamingr seemed to have some concern that isn't obvious, and it may make sense to wait for a reply before making a doc change

@apapirovski
Copy link
Member

apapirovski commented Apr 11, 2018

@reviwher I think they had the same first look thought which is that it mentions TypedArray but of course on closer inspection it refers to the .buffer property. Much like you already highlighted above. I think if someone can come up with a sensible PR then this could get fixed quickly.

@Trott Trott closed this as completed in 2652ab3 Apr 20, 2018
jasnell pushed a commit that referenced this issue Apr 20, 2018
Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

PR-URL: #19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 29, 2018
Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

Backport-PR-URL: #21590
PR-URL: #19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 9, 2018
Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

Backport-PR-URL: #21590
PR-URL: #19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 10, 2018
Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

Backport-PR-URL: #21590
PR-URL: #19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
rvagg pushed a commit that referenced this issue Aug 16, 2018
Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

Backport-PR-URL: #21590
PR-URL: #19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
rvagg pushed a commit that referenced this issue Aug 16, 2018
Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

Backport-PR-URL: #21590
PR-URL: #19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Buffer.from / new Buffer accept Uint8Array

Fixes: nodejs/node#14118

PR-URL: nodejs/node#19949
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests

4 participants