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: Uint8Array support in Buffer functions #19948

Closed
wants to merge 1 commit into from
Closed

doc: Uint8Array support in Buffer functions #19948

wants to merge 1 commit into from

Conversation

reviewher
Copy link

Buffer.from / new Buffer accept Uint8Array

Fixes: #14118

Checklist

@nodejs-github-bot 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
@@ -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` to copy data from.
Copy link
Member

@jasnell jasnell Apr 11, 2018

Choose a reason for hiding this comment

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

This should likely be Buffer|TypedArray with the description An existing BufferorTypedArray` instance to copy from.

It's worth noting that any TypedArray can be used, not just Uint8Array. The issue, however, is that as values are copied, each will be coerced to an 8-byte value. For instance, Buffer.from(new Uint32Array([0xffff, 2, 3]) will produce a Buffer with values [0xff, 2, 3]. Likewise, Buffer.from(new Float32Array([1.23, 2, 3])) will yield a Buffer with values [1, 2, 3].

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell That’s more or less already documented as https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_array – we accept anything with a length property, basically.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I meant just worth noting in this thread, not in the docs. My mistake, I wasn't clear about that at all. The change here that I'd like to see is just listing TypedArray instead of Uint8Array in the bullet point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell the body language may need some clarification if the signature mentions general Typed Arrays:

Copies the passed buffer data onto a new Buffer instance.

For a typed array that isn't a Uint8Array, does the "data" refer to the typed values or the .buffer data? e.g.

Buffer.from(new Float32Array([Math.PI]))        // <Buffer 03>
Buffer.from(new Float32Array([Math.PI]).buffer) // <Buffer db 0f 49 40>

This issue is obviated in the Uint8Array case but is relevant for other typed arrays.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, exactly: Other kinds of TypedArrays are treated just like regular Arrays. Hence my comment above – I think it would make more sense to document TypedArray as part of the Buffer.from(array) heading.

Copy link
Member

Choose a reason for hiding this comment

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

The Buffer.from(new Float32Array([Math.PI]).buffer) variant is already covered in the documentation by the Class Method: Buffer.from(arrayBuffer[, byteOffset[, length]]) section. In that specific case, when an ArrayBuffer instance is passed, the Buffer returned is a view over the existing ArrayBuffer and no data is copied or coerced.

The Buffer.from(new Float32Array([Math.PI])) case is covered by the Class Method: Buffer.from(buffer) variant but the doc for that specific variant only needs to be indicate that any TypedArray can be passed but that each typed value will be copied and coerced into 8-bit values in the returned Buffer.

The Class Method: Buffer.from(buffer) and Class Method: Buffer.from(array) variants can be merged into a single section as the behavior for each is generally identical.

Copy link
Author

Choose a reason for hiding this comment

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

Local state was messed up, resubmitted PR in #19949 . I agree with @addaleax 's suggestion to clarify the typed array situation elsewhere, but that is a separate discussion

Copy link
Member

Choose a reason for hiding this comment

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

Also note that Buffer.from(new Uint8Array([1,2,3])) is treated exactly the same as Buffer.from([1,2,3]) ... that is, the new Buffer contains a copy of the original.

while Buffer.from((new Uint8Array([1,2,3])).buffer) returns a new Buffer that contains a view over the same backing ArrayBuffer. No data is copied.

Buffer.from / new Buffer accept Uint8Array (fixes #14118)
@reviewher
Copy link
Author

Github state is messed up, will submit a new PR

@reviewher reviewher closed this Apr 11, 2018
@reviewher reviewher deleted the patch-1 branch April 11, 2018 14:49
@addaleax
Copy link
Member

@reviewher I think this PR was fine as it is, if you want?

@reviewher
Copy link
Author

@addaleax there was a missing link to the Uint8Array page. Tried to fix it and GitHub wasn't updating the commit, hence the resubmission in #19949

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants