Skip to content

stream: corrections to source normalization - #63416

Closed
Renegade334 wants to merge 1 commit into
nodejs:mainfrom
Renegade334:stream-iter-source-normalization
Closed

stream: corrections to source normalization#63416
Renegade334 wants to merge 1 commit into
nodejs:mainfrom
Renegade334:stream-iter-source-normalization

Conversation

@Renegade334

Copy link
Copy Markdown
Member
  • Arrays, as special cases of sync iterables, have lower precedence than async iterables (and protocol objects) in async normalization operations.
  • Sync iterables should be normalized synchronously in async normalization operations, ie. they may not contain nested async iterables (continuation of stream: minor stream/iter implementation edits #63132).
  • Similarly, sync toStreamables should not be allowed to return async sources.
  • No explicit rejection is required for async patterns in fromSync(); doing so may exclude objects with both sync and async functionality (eg. objects with both [Symbol.iterator] and [Symbol.asyncIterator] properties). The spec text seems to suggest that objects with a valid sync pattern should be accepted here even if they also have an async pattern, but could do with some clarity.
  • In from(), normalise the object returned by toAsyncStreamable directly, not the return value of its Symbol.asyncIterator method: firstly, the async iterator returned by the Symbol.asyncIterator method is not itself obliged to have a Symbol.asyncIterator method, and secondly, the result could in theory be any valid from() source, not just an async iterable.
  • Use isUint8ArrayBatch for the from()/fromSync() fast paths.

Note that as things stand, some of the from() cases throw validation errors synchronously (eg. fromSync({ [toStreamable]() {} })) whereas the majority only throw validation errors at the point of iteration. AFAICT, the latter behaviour is what is compliant with the spec, but I haven't touched these for now.

Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@Renegade334
Renegade334 requested a review from jasnell May 18, 2026 17:45
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels May 18, 2026
@codecov

codecov Bot commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.19658% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.03%. Comparing base (3b19867) to head (5da7742).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/streams/iter/from.js 81.19% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63416      +/-   ##
==========================================
- Coverage   91.68%   90.03%   -1.65%     
==========================================
  Files         361      714     +353     
  Lines      156230   225741   +69511     
  Branches    24021    42710   +18689     
==========================================
+ Hits       143232   203242   +60010     
- Misses      12729    14298    +1569     
- Partials      269     8201    +7932     
Files with missing lines Coverage Δ
lib/internal/streams/iter/from.js 81.47% <81.19%> (-5.36%) ⬇️

... and 476 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasnell

jasnell commented Aug 14, 2026

Copy link
Copy Markdown
Member

@Renegade334 ... I'm just now finally getting to clear my backlog of reviews. Can I ask you to rebase this then I'll give it a review?

@Renegade334

Copy link
Copy Markdown
Member Author

I think this is probably well out-of-state after other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants