Skip to content

stream: use validateString for consumer encoding - #64754

Open
sjungwon03 wants to merge 1 commit into
nodejs:mainfrom
sjungwon03:stream/use-validate-string-consumer-encoding
Open

stream: use validateString for consumer encoding#64754
sjungwon03 wants to merge 1 commit into
nodejs:mainfrom
sjungwon03:stream/use-validate-string-consumer-encoding

Conversation

@sjungwon03

@sjungwon03 sjungwon03 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

This replaces the manual options.encoding type check in stream iterator
consumers with the shared validateString() validator.

- if (typeof options.encoding !== 'string') {
-   throw new ERR_INVALID_ARG_TYPE('options.encoding', 'string',
-                                  options.encoding);
- }
+ validateString(options.encoding, 'options.encoding');

This preserves the existing error behavior while using the common validation
helper. It also adds coverage for non-string encoding values in both the
async text() and sync textSync() consumers.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@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 Jul 26, 2026
@sjungwon03
sjungwon03 marked this pull request as ready for review July 26, 2026 09:38
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (4a5eb1c) to head (396f764).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #64754   +/-   ##
=======================================
  Coverage   90.15%   90.15%           
=======================================
  Files         744      744           
  Lines      242517   242539   +22     
  Branches    45688    45690    +2     
=======================================
+ Hits       218642   218671   +29     
- Misses      15358    15368   +10     
+ Partials     8517     8500   -17     
Files with missing lines Coverage Δ
lib/internal/streams/iter/consumers.js 96.63% <100.00%> (-0.02%) ⬇️

... and 29 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.

@mertcanaltin mertcanaltin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Replace the manual `options.encoding` type check in stream iterator
consumers with the shared validator. Add coverage for non-string values
in the async and sync text consumers.

Signed-off-by: sjungwon03 <sjungwon03@gmail.com>
@sjungwon03
sjungwon03 force-pushed the stream/use-validate-string-consumer-encoding branch from 4913e0f to 396f764 Compare July 28, 2026 14:04
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.

5 participants