-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
stream: allow stream to stay open after take #47023
base: main
Are you sure you want to change the base?
Changes from 5 commits
ff564fd
abce0a7
df52ac1
2f38460
d52c8ab
c8a4849
8af907f
444cf40
6e09304
2542401
1ae2061
d3fc3c6
55907e6
99322a8
3fbdf40
152e60b
1b8fa15
4ea9d73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ const { | |
validateAbortSignal, | ||
validateInteger, | ||
validateObject, | ||
validateBoolean, | ||
} = require('internal/validators'); | ||
const { kWeakHandler } = require('internal/event_target'); | ||
const { finished } = require('internal/streams/end-of-stream'); | ||
|
@@ -397,13 +398,16 @@ function take(number, options = undefined) { | |
if (options?.signal != null) { | ||
validateAbortSignal(options.signal, 'options.signal'); | ||
} | ||
if (options?.destroyStream != null) { | ||
validateBoolean(options.destroyStream, 'options.destroyStream'); | ||
} | ||
|
||
number = toIntegerOrInfinity(number); | ||
return async function* take() { | ||
if (options?.signal?.aborted) { | ||
throw new AbortError(); | ||
} | ||
for await (const val of this) { | ||
for await (const val of this.iterator({ destroyOnReturn: options?.destroyStream ?? true })) { | ||
if (options?.signal?.aborted) { | ||
throw new AbortError(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that if the stream fails we should close the stream, WDYT @ronag ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mcollina any thoughts on your side? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another name would be better? Not sure what though... @benjamingr