stream: allow stream to stay open after take by rluvaton · Pull Request #47023 · nodejs/node · GitHub

fix #46980

TODO

add tests
add docs

fix nodejs#46980

Review requested:

@nodejs/streams

Maybe another name would be better? Not sure what though... @benjamingr

I think that if the stream fails we should close the stream, WDYT @ronag ?

@mcollina any thoughts on your side?

after thinking I think it should be closed on an error as in the iterator helpers proposal spec the underlying iterator should be closed when it failed

Thanks for opening a PR! Can you please add a unit test?

Docs are also missing

Hey @mcollina I've added tests and docs :)

lgtm

@mcollina is something holding this back from merging? Or we waiting for the TC39?

we have 48 hours grace period to allow for folks across the globe to review

Failed to start CI

- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 403 Forbidden

https://github.com/nodejs/node/actions/runs/4384604326

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

Co-authored-by: Debadree Chatterjee <debadree333@gmail.com>

…fter-take-46980' into feat/allow_stream-to-stay-open-after-take-46980

I think you would need to remove the merge commit since supposedly it can cause issues with the tooling everything else is good!!

I think you would need to remove the merge commit since supposedly it can cause issues with the tooling everything else is good!!

aren't we using squash and merge?

I think you would need to remove the merge commit since supposedly it can cause issues with the tooling everything else is good!!

aren't we using squash and merge?

Ref: #46910 (comment)

I don't think discussion has exhausted itself in the spec issue and I don't want to deviate from it until we proved we must

I don't think discussion has exhausted itself in the spec issue and I don't want to deviate from it until we proved we must

Should be add the blocked label for now then?

Yeah sorry I meant to explicitly block and hit the wrong button

I think what you really want is a chunk (which will be proposed for inclusion in standard iterators in the future) or a nextN helper like

function nextN(iterator, n) {
  const result = [];
  const next = iterator.next;
  for (; n > 0; --n) {
    cont { done, value } = next.call(iterator);
    if (done) break;
    result.push(value);
  }
  return result;
}

@michaelficarra no problem but maybe could you update the proposal first so we won't have this back and forth

FYI I think nextN is better name than chunk

@rluvaton chunk is a different method than the nextN that I wrote. See tc39/proposal-iterator-helpers#71 (comment) or https://www.npmjs.com/package/chunk.

@rluvaton chunk is a different method than the nextN that I wrote. See tc39/proposal-iterator-helpers#71 (comment) or npmjs.com/package/chunk.

@michaelficarra

Oh, after looking at chunk what I really want is nextN which is basically a non-closing take

Do you think that non-closing take will be added?

chunk is less what I want as:

I don't want chunks I just want the x items and the rest left unchanged (like in the headers row from CSV file - explained more in the linked issue)
The data is not evenly distributed
It won't let me get X items without closing the stream, I would just get another iterator that emits in chunks which I can't exit as it will close

Having nextN as an operator that returns iterable would enable:

better chaining
better efficiency (no need to save the values in case we don't need - without .toArray())

Example of usages:

having it as an operator:

const responses = await topVisitedUrlsIterator
	.filter(noEmptyLine)
	.drop(1) // Header
	.nextN(8)
	.map(parseLine)
	.map(value => apiCall(value), { concurrency: 4 })
	.toArray();

having it as a static function:

const values = await nextN(
  topVisitedUrlsIterator
    .filter(noEmptyLine)
    .drop(1),
  8)
  .map(parseLine);

// How would I use the concurrency that we would have in the map?
const responses = concurrentMap(values, (value) => apiCall(value), 4);

# Conflicts: # lib/internal/streams/operators.js

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

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

rluvaton added 2 commits March 9, 2023 00:34

stream: allow stream to stay open after take

ff564fd

fix nodejs#46980

stream: close stream on error

abce0a7

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 9, 2023

rluvaton mentioned this pull request Mar 9, 2023

stream: Add option to Readable.take operator to not close the stream #46980

Open

rluvaton added 2 commits March 9, 2023 08:41

stream: remove unnecessary code

df52ac1

stream: add import

2f38460

ronag approved these changes Mar 9, 2023

View reviewed changes

ronag reviewed Mar 9, 2023

View reviewed changes

stream: fix when options is missing

d52c8ab

rluvaton commented Mar 9, 2023

View reviewed changes

mcollina requested changes Mar 9, 2023

View reviewed changes

rluvaton added 7 commits March 9, 2023 14:58

stream: added tests

c8a4849

stream: fail in stream will close the stream

8af907f

stream: fix taking item from next iteration

444cf40

stream: format

6e09304

stream: use destructure

2542401

stream: trigger ci

1ae2061

stream: add docs

d3fc3c6

mcollina approved these changes Mar 10, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 10, 2023

debadree25 reviewed Mar 12, 2023

View reviewed changes

Comment thread doc/api/stream.md Outdated

Update doc/api/stream.md

55907e6

Co-authored-by: Debadree Chatterjee <debadree333@gmail.com>

debadree25 reviewed Mar 12, 2023

View reviewed changes

Comment thread doc/api/stream.md Outdated

rluvaton added 2 commits March 12, 2023 11:13

stream: update docs

99322a8

Merge remote-tracking branch 'origin/feat/allow_stream-to-stay-open-a…

3fbdf40

…fter-take-46980' into feat/allow_stream-to-stay-open-after-take-46980

debadree25 approved these changes Mar 12, 2023

View reviewed changes

stream: update docs

152e60b

benjamingr reviewed Mar 12, 2023

View reviewed changes

ronag added the blocked PRs that are blocked by other issues or PRs. label Mar 12, 2023

benjamingr requested changes Mar 12, 2023

View reviewed changes

michaelficarra reviewed Mar 15, 2023

View reviewed changes

rluvaton added 2 commits May 6, 2023 23:25

Merge branch 'main' into feat/allow_stream-to-stay-open-after-take-46980

1b8fa15

# Conflicts: # lib/internal/streams/operators.js

fix conflict

4ea9d73

rluvaton mentioned this pull request May 20, 2023

stream: unref #48007

Closed

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Aug 8, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023

github-actions Bot added the stale label Jul 28, 2026

coderabbitai Bot mentioned this pull request Jul 30, 2026

fix(stream): complete Node.js 26.5 stream parity PerryTS/perry#7067

Merged

14 tasks

avivkeller closed this Aug 9, 2026

+                if (options?.destroyStream != null) {
+                  validateBoolean(options.destroyStream, 'options.destroyStream');
+                }

+                  for await (const val of this.iterator({ destroyOnReturn: options?.destroyStream ?? true })) {
                     if (options?.signal?.aborted) {
                       throw new AbortError();
                     }

stream: allow stream to stay open after take by rluvaton · Pull Request #47023 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: allow stream to stay open after take - #47023

stream: allow stream to stay open after take#47023rluvaton wants to merge 18 commits intonodejs:mainnodejs/node:mainfrom rluvaton:feat/allow_stream-to-stay-open-after-take-46980rluvaton/node:feat/allow_stream-to-stay-open-after-take-46980Copy head branch name to clipboard

Conversation

rluvaton commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

nodejs-github-bot commented Mar 9, 2023

Uh oh!

ronag Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

rluvaton Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

rluvaton Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

rluvaton Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 9, 2023

Uh oh!

rluvaton commented Mar 9, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Mar 10, 2023

Uh oh!

mcollina commented Mar 10, 2023

Uh oh!

github-actions Bot commented Mar 10, 2023

Uh oh!

nodejs-github-bot commented Mar 11, 2023

Uh oh!

Uh oh!

Uh oh!

debadree25 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Mar 12, 2023

Uh oh!

debadree25 commented Mar 12, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Mar 12, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

michaelficarra left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Mar 15, 2023

Uh oh!

michaelficarra commented Mar 15, 2023

Uh oh!

rluvaton commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rluvaton commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 10, 2023

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

stream: allow stream to stay open after take#47023
rluvaton wants to merge 18 commits into
nodejs:mainfrom
rluvaton:feat/allow_stream-to-stay-open-after-take-46980

rluvaton commented Mar 9, 2023 •
edited
Loading

debadree25 left a comment •
edited
Loading

rluvaton commented Mar 16, 2023 •
edited
Loading

rluvaton commented Mar 16, 2023 •
edited
Loading