doc: Uint8Array support in Buffer functions #19949

SheetJSDev · 2018-04-11T14:52:40Z

Buffer.from / new Buffer accept Uint8Array

Checklist

documentation is changed or added
commit message follows commit guidelines

jasnell · 2018-04-11T15:08:51Z

doc/api/buffer.md

@@ -402,7 +402,7 @@ changes:

 > Stability: 0 - Deprecated: Use [`Buffer.from(buffer)`] instead.

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


I would make this {Buffer|TypedArray} and An existing BufferorTypedArray` to copy data from.

Still not a fan of that… as you noted in the other PR, this has semantics which match Buffer.from(array), which we document as a different overload, and similarly has a completely different performance profile from Uint8Array.

I’d be okay with it if we were to merge those two headings together, but that seems out of scope for this PR.

@addaleax The problem with citing Buffer.from(array) is the last line:

A TypeError will be thrown if array is not an Array.

Typed arrays aren't Arrays :/

If it is agreed that Buffer.from should accept general typed arrays, the behavior should be documented. It might make sense to just introduce a new section Buffer.from(typedarray). In any case, I think that's outside the scope of this PR.

A TypeError will be thrown if array is not an Array.

Well that’s just a plain lie in the docs. 😄 Even Buffer.from({length: 4}) works perfectly – really all that matters is that .length is not undefined. If you prefer it, that can also be taken care of at a later point/by somebody else.

Probably better to do it later with fresh eyes. The other Buffer functions should also be revisited.

vsemozhetbyt

@SheetJSDev Thank you!
Mentioned nits can be fixed by a lander if it is not convenient for you)

vsemozhetbyt · 2018-04-11T15:07:12Z

doc/api/buffer.md

@@ -842,7 +842,7 @@ A `TypeError` will be thrown if `arrayBuffer` is not an [`ArrayBuffer`] or a
 added: v5.10.0
 -->

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


Missing period: from -> from.

to coy data from -> from which to copy data

vsemozhetbyt · 2018-04-11T15:10:12Z

doc/api/buffer.md

@@ -402,7 +402,7 @@ changes:

 > Stability: 0 - Deprecated: Use [`Buffer.from(buffer)`] instead.

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


This doc has many violations of 'wrap after 80 characters' rule and we make an exception for it now, but maybe it is worth to not add more. So this and next changed line can be wrapped.

@vsemozhetbyt after facing issues earlier today with github not refreshing PRs, it's probably better for a lander to amend the commit.

to copy data from -> from which to copy data

Trott

Hi, @SheetJSDev! Welcome and thanks for the PR! Documentation PRs can attract a lot of comments. Don't get discouraged!

SheetJSDev · 2018-04-12T06:12:53Z

Updated commit, incorporating @Trott 's recommendation as well as reworking lines that violated the 80-character limit.

@jasnell @addaleax on second look, the best way to resolve the general Typed Array case is to modify the Buffer.from(array) text, since the array case applies the same sort of coercion:

> Buffer.from([Math.PI])
<Buffer 03>
> Buffer.from(new Float32Array([Math.PI]))
<Buffer 03>

One possible definition:

### Class Method: Buffer.from(array)

* `array` {array-like}

Allocates a new `Buffer` of length `array.length`.  Values from `array` are
converted to unsigned 8-bit integers.

<example here>

Given that general TypedArrays are array-like, it would be covered in this case.

jasnell · 2018-04-12T16:04:49Z

I'm good with that!

Trott · 2018-04-12T20:15:35Z

@nodejs/documentation What should we do about the type? I don't think adding {array-like} passes the smell test for me. We don't do that anywhere else in our docs. But what should we do in this case?

Trott · 2018-04-12T20:44:26Z

doc/api/buffer.md

@@ -81,7 +80,7 @@ to one of these new APIs.*

 * [`Buffer.from(array)`] returns a new `Buffer` that *contains a copy* of the
  provided octets.
-* [`Buffer.from(arrayBuffer[, byteOffset [, length]])`][`Buffer.from(arrayBuffer)`]
+* [`Buffer.from(arrayBuffer[, byteOffset[, length]])`][`Buffer.from(arrayBuf)`]


The whitespace change on this line makes this formatted differently from all our other docs. This change appears to be just to keep things to wrapping at 80 characters. I would undo it and not worry about the 80 character wrapping on this line in this PR. It's good to take care of lines that you are already editing for other reasons, and we often encourage doing so for nearby lines too. But to add 80-char wrapping to the entire doc with this otherwise narrow change is probably not the way to go. For one thing, it will make backporting more difficult.

Was not this a typo space in this exact case? Space before comma seems uncommon. There were some in this doc, but not everywhere.

@vsemozhetbyt Yes, you're right. Not sure about shortening arrayBuffer to arrayBuf but then again, we already use buf elsewhere for a Buffer instance, so why not? :-D

I'm convinced the whitespace was in error. If you search https://nodejs.org/api/all.html for [,, you will find 9 instances, corresponding to this function and to socket.send(msg, [offset, length,] port [, address] [, callback]). I think the socket.send case has whitespace because both address and callback are optional, and it would be very confusing to read without the whitespace.

The shortened arrayBuf occurs in the link marker, not in the actual body text, so it won't affect the HTML output. IMHO it's an innocuous change.

Yup, I'm on board with this now. Sorry for the noise.

Trott

Left some micro-nits but looks good. Really close, I think.

SheetJSDev · 2018-04-12T20:53:20Z

@Trott feel free to tinker with the commit.

On the issue of "array-like", Buffer.from knows about array-like objects: https://github.com/nodejs/node/blob/master/lib/buffer.js#L234

You can elicit that error with the following corner case:

> Buffer.from(({valueOf:()=>{}}))
TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type object

Searching in the source tree reveals that it is the only error message that uses the phrase "Array-like Object":

$ grep "Array-like Object" -RiIl lib
lib/buffer.js

Trott · 2018-04-12T21:01:12Z

@SheetJSDev I'm OK with the term array-like in prose. What gives me pause is it's use as a type. The types we use are finite: {any}, {string}, {object}, etc. We don't use {array-like} in the listings of types for arguments anywhere else, so this would be a first. And that seems potentially problematic to me. But maybe that's just me. I suspect it's also the only place we'd be using an adjective for a type rather than a noun. It seems awkward to me, but I can live with it if it's the best descriptor we can come up with that accurately communicates what's going on.

vsemozhetbyt · 2018-04-12T21:17:19Z

FWIW, MDN has this:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Indexed_collections

Trott · 2018-04-14T22:58:13Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/525/

devsnek

nice to see you around friend :D

Buffer.from / new Buffer accept Uint8Array Fixes: #14118

Trott · 2018-04-20T17:52:15Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/558/

Trott · 2018-04-20T18:07:31Z

Landed in 2652ab3.

Thanks for the contribution! 🎉

Buffer.from / new Buffer accept Uint8Array Fixes: nodejs#14118 PR-URL: nodejs#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]>

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]>

ChALkeR · 2018-06-29T17:03:18Z

@nodejs/lts @MylesBorins It might be worth to land this on v8.x LTS.
The behavior is explicitly there since beca324 (but it also worked before that).

MylesBorins · 2018-06-29T18:15:23Z

@ChALkeR the commit does not land cleanly, could you open a backport?

ChALkeR · 2018-06-29T19:13:33Z

@MylesBorins #21590 — will that do?

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]>

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]>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Apr 11, 2018

reviewher mentioned this pull request Apr 11, 2018

doc: Uint8Array support in Buffer functions #19948

Closed

2 tasks

addaleax approved these changes Apr 11, 2018

View reviewed changes

vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 11, 2018

jasnell reviewed Apr 11, 2018

View reviewed changes

addaleax removed the fast-track PRs that do not need to wait for 48 hours to land. label Apr 11, 2018

vsemozhetbyt approved these changes Apr 11, 2018

View reviewed changes

Trott reviewed Apr 12, 2018

View reviewed changes

benjamingr approved these changes Apr 12, 2018

View reviewed changes

jasnell approved these changes Apr 14, 2018

View reviewed changes

devsnek approved these changes Apr 15, 2018

View reviewed changes

doc: Uint8Array support in Buffer functions

3ffe707

Buffer.from / new Buffer accept Uint8Array Fixes: #14118

Trott closed this Apr 20, 2018

MylesBorins added the backport-requested-v8.x label Jun 29, 2018

ChALkeR mentioned this pull request Jun 29, 2018

[v8.x backport] doc: Backport #19685 and #19949 to v8.x #21590

Closed

2 tasks

MylesBorins added backported-to-v8.x and removed backport-requested-v8.x labels Jul 9, 2018

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Uint8Array support in Buffer functions #19949

doc: Uint8Array support in Buffer functions #19949

SheetJSDev commented Apr 11, 2018

jasnell Apr 11, 2018

addaleax Apr 11, 2018

SheetJSDev Apr 11, 2018

addaleax Apr 11, 2018

SheetJSDev Apr 11, 2018

vsemozhetbyt left a comment

vsemozhetbyt Apr 11, 2018

Trott Apr 12, 2018

vsemozhetbyt Apr 11, 2018

SheetJSDev Apr 11, 2018

Trott Apr 12, 2018

Trott left a comment

SheetJSDev commented Apr 12, 2018

jasnell commented Apr 12, 2018

Trott commented Apr 12, 2018

Trott Apr 12, 2018

vsemozhetbyt Apr 12, 2018 •

edited

Loading

Trott Apr 12, 2018

SheetJSDev Apr 12, 2018

Trott Apr 12, 2018

Trott left a comment

SheetJSDev commented Apr 12, 2018

Trott commented Apr 12, 2018

vsemozhetbyt commented Apr 12, 2018

Trott commented Apr 14, 2018

devsnek left a comment

Trott commented Apr 20, 2018

Trott commented Apr 20, 2018

ChALkeR commented Jun 29, 2018

MylesBorins commented Jun 29, 2018

ChALkeR commented Jun 29, 2018

doc: Uint8Array support in Buffer functions #19949

doc: Uint8Array support in Buffer functions #19949

Conversation

SheetJSDev commented Apr 11, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

SheetJSDev commented Apr 12, 2018

jasnell commented Apr 12, 2018

Trott commented Apr 12, 2018

Choose a reason for hiding this comment

vsemozhetbyt Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

SheetJSDev commented Apr 12, 2018

Trott commented Apr 12, 2018

vsemozhetbyt commented Apr 12, 2018

Trott commented Apr 14, 2018

devsnek left a comment

Choose a reason for hiding this comment

Trott commented Apr 20, 2018

Trott commented Apr 20, 2018

ChALkeR commented Jun 29, 2018

MylesBorins commented Jun 29, 2018

ChALkeR commented Jun 29, 2018

vsemozhetbyt Apr 12, 2018 •

edited

Loading