stream: add toArray helpers

[benjamingr]

This is the next one and should only land after #41445 does (I'll rebase when it does).

Marking as draft to not take CI resources until that one lands but I'd like reviews :)

This adds the toArray method from iterator helpers and addresses a common use case.

My primary concern is that people don't over-use/abuse it though this is already pretty common with toArray and others on npm with many downloads.

I don't remember if this was in the initial work with @ronag on map so I added a co-authored-by to be on the safe side though I assume this granularity of attribution doesn't really matter to either of us very much.

cc @ronag @nodejs/streams

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[Mesteery]

@mscdex changed the title stream: add toArray helper stream: add forEach and toArray helpers

This just depends on #41445:

This is the next one and should only land after #41445 does (I'll rebase when it does).

[benjamingr]

(Meta: if there is a clearer way to open such a PR let me know - I can target that branch instead of master to make the fact it's "on top" of another PR clearer, honestly I am just frustrated the ARM build has been taking hours + failing today a bunch)

[aduh95]

Suggested change

	async function toArray(options) {
	/**
	* @param {{ signal?: AbortSignal }} [options]
	* @returns {Array<unknown> | Buffer}
	*/
	async function toArray(options = undefined) {

[Mesteery]

Suggested change

	async function toArray(options) {
	/**
	* @param {{ signal?: AbortSignal }} [options]
	* @returns {Buffer | any[]}
	*/
	async function toArray(options = undefined) {

For simple types, ...[] is more common than Array<...> in the codebase (same for any).

[benjamingr]

We don't have JSDoc comments for any of the other functions in this file, should we maybe do a PR adding it to all of them at once?

[aduh95]

I'm fine with JSDoc not being included. We probably want toArray.length to be 0, to align with the proposal.

Suggested change

	async function toArray(options) {
	async function toArray(options = undefined) {

[benjamingr]

@aduh95 the .length is different in all the other methods (that take a signal), we've opened an issue to ensure forward-compatibility to ensure that.

Given the primary proposal contributors from what I can tell @devsnek and @ExE-Boss are node contributors I will happily accept guidance with regards to how strict we should be about it :)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41939/

[benjamingr]

@mcollina I think this is ready for review

[mcollina]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41948/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41958/

[aduh95]

nit: you can avoid creating intermediary arrays (and imho it's easier to read, but feel free to disagree)

Suggested change

	Array(100).fill().map((_, i) => i),
	Array.from({ length: 100 }, (_, i) => i),

[jasnell]

I'd prefer if this used a Uint8Array instead of Buffer.

[benjamingr]

@jasnell .read() returns a buffer so I assumed this should return the same type. As buffers are all Uint8Arrays and this behavior is stream specific I'm not sure what we gain?

Why do you prefer Uint8Array here? Is there some long-term plan to minimize/deprecate Buffer as a type?

[jasnell]

Why do you prefer Uint8Array here? Is there some long-term plan to minimize/deprecate Buffer as a type?

Consider it a non-blocking personal preference to start moving away, as much as possible, from Node.js specific APIs where there is a language and web platform standard API option available. We can't change the existing stream.Readable API but we don't have to introduce new dependencies on the implementation specific APIs.

[benjamingr]

@ronag @mcollina thoughts?

[sindresorhus]

Consider it a non-blocking personal preference to start moving away, as much as possible, from Node.js specific APIs where there is a language and web platform standard API option available.

I strongly agree with this too, as a heavy user of Node.js. Buffer makes it harder to create cross-platform packages.

I think this merits a larger discussion in a dedicated issue though. I'd like to see an official stance on this, for Buffer specifically, but also generally.

[benjamingr]

@sindresorhus @jasnell opened #41588 - since this API is experimental I am content with shipping it with buffer and breaking it in a small way in the next version.

[ronag]

What does the spec say/do here?

[mcollina]

The rule of least surprise would tell me to return a Buffer here. I don't think we should return a UInt8Array unless it's specified by the spec.

[benjamingr]

@ronag If we were to toArray an async iterator it would return a list of all the chunks I would assume it would not concat them. I can change the behavior to an array of buffers but I am not sure it's particularly useful to users.

I suspect/hope that web streams would overload .toArray to do what users would expect (return the stream as an array) rather than a list of chunks.

[benjamingr]

This behavior is probably changing and we are going with no non-objectMode specific behavior #41615 - it's also good to Note toArray will always return well... an array :)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41980/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41992/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41995/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42004/