Uh oh!
There was an error while loading. Please reload this page.
stream: add dump()/dumpSync() for stream/iter - #65598
Conversation
nodejs-github-bot
commented
Aug 27, 2026
Review requested:
|
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #65598 +/- ##
==========================================
- Coverage 90.17% 90.15% -0.02%
==========================================
Files 769 769 Lines 261448 261545 +97 Branches 49674 49668 -6 ==========================================
+ Hits 235759 235806 +47 - Misses 16736 16775 +39 - Partials 8953 8964 +11
🚀 New features to boost your workflow:
|
nodejs-github-bot
commented
Aug 27, 2026
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.
Ethan-Arrowood
commented
Aug 28, 2026
I'm okay with considering a different name. Honestly, I stuck with Let the bikeshedding begin! |
jasnell
commented
Aug 29, 2026
|
Ethan-Arrowood
commented
Aug 31, 2026
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
e1e8ca0CompareEthan-Arrowood
commented
Sep 4, 2026
We agreed on |
a24f864 to
ccf6409Comparenodejs-github-bot
commented
Sep 4, 2026
nodejs-github-bot
commented
Sep 4, 2026
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Ethan-Arrowood
commented
Sep 4, 2026
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
2db8a62CompareEthan-Arrowood
commented
Sep 4, 2026
My bad @pimterry . I manually reviewed the updates now for any erroneous find/replaces, looks all good to me. |
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!