stream: add dump()/dumpSync() for stream/iter - #65598
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
Ethan-Arrowood
added
the
quic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65598 +/- ##
==========================================
- Coverage 90.07% 90.06% -0.02%
==========================================
Files 751 751
Lines 254881 255069 +188
Branches 48111 48177 +66
==========================================
+ Hits 229582 229720 +138
- Misses 16486 16547 +61
+ Partials 8813 8802 -11
🚀 New features to boost your workflow:
|
Ethan-Arrowood
added
the
request-ci
github-actions
Bot
removed
the
request-ci
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Very onboard with the concept, implementation looks good, small point on a test case.
Larger question though: can we rename this? Drain is a separate related but different concept on the writer API. Giving them the same name is a bit confusing, and hits practical issues as well (like the various tests here that have to rename existing drain references that mean something else).
dump() or discard()?
| // drain: does not retain data | ||
| // ============================================================================= | ||
|
|
||
| async function testDrainDoesNotRetain() { |
There was a problem hiding this comment.
This doesn't really do what it says it does - as is it roughly duplicates testDrainConsumesToCompletion I think. It allocates the buffer once outside the source, so it can't check what's retained anyway. Needs a buffer per chunk with a WeakRef & GC dance if you want to test this I think.
|
I'm okay with considering a different name. Honestly, I stuck with Let the bikeshedding begin! |
|
|
|
I feel like consume implies its being used somehow. Like all the other methods "consume" the stream and then return something. I think Can people +1 this comment if they agree on |
2e586f3 to
e1e8ca0
Compare
Ethan-Arrowood
changed the title
|
We agreed on |
a24f864 to
ccf6409
Compare
github-actions
Bot
removed
the
request-ci
github-actions
Bot
removed
the
request-ci
|
Good catch, fixing now. |
Every consumer in node:stream/iter retains what it reads, so there is
no way to read a streamable to completion while keeping nothing. To
clear a stream, callers write `for await (const _ of source) {}`. This
is apparent throughout multiple test suites, including the stream/iter
tests themselves.
These are especially useful for QUIC where a receiver doesn't want the
payload, but still has to read it to completion to relieve
backpressure. bytes() does that too, but allocates the whole payload to
discard it.
dump() pulls every batch and drops it, so peak memory is one batch
regardless of volume. It takes the same signal and limit options as
the other consumers, rejects if the source errors mid-stream, and
fulfills with undefined. dumpSync() is the synchronous form.
The name follows undici's body.dump(). drain() was the obvious
alternative but this module already exports ondrain() and
drainableProtocol() for writer-side backpressure, which is the
opposite side of the pipe.
Assisted-by: Claude Opus 5
Signed-off-by: Ethan Arrowood <ethan@arrowood.dev>
Replaces the `for await (const _ of source) {}` discard idiom with
dump(), which says what it does and does not need an eslint-disable
for the unused loop variable. 89 loops across 60 files, plus 32 now
redundant eslint-disable directives and comments removed.
Only files that already declare --experimental-stream-iter are
converted, so no test gains an experimental flag it did not already
opt into. That leaves 21 files with the old idiom; converting those
would change what those tests run under and belongs in a separate
discussion.
Loops that break early are left alone: those cancel the source rather
than reading it to completion, which is not what dump() does.
Assisted-by: Claude Opus 5
Signed-off-by: Ethan Arrowood <ethan@arrowood.dev>
ccf6409 to
2db8a62
Compare
|
My bad @pimterry . I manually reviewed the updates now for any erroneous find/replaces, looks all good to me. |
8 participants
Adds
dump()anddumpSync()tonode:stream/iter.This will enable us to replace
for awaitloops withawait dump(stream)across the repo.I used two commits: one for the feature, another for the migration so it's easy to review.
I only migrated tests that were already using iter streams. But there are more that could benefit from this util.
This api is inspired by things like Undici's
body.dump()method.Let me know what you think of this API addition!