Skip to content
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: handle undefined chunks correctly in decode stream #55153

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

Nahee-Park
Copy link
Contributor

Align TextDecoderStream behavior with WPT requirements by treating undefined chunks as errors. This change ensures that TextDecoderStream properly handles unexpected chunk types and throws an error when receiving undefined input.

This update addresses the failing WPT for decode stream error handling.

Align TextDecoderStream behavior with WPT requirements by treating
undefined chunks as errors. This change ensures that TextDecoderStream
properly handles unexpected chunk types and throws an error when
receiving undefined input.

This update addresses the failing WPT for decode stream error handling.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Sep 28, 2024
Copy link

codecov bot commented Sep 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (e32521a) to head (cc89cf2).
Report is 343 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55153      +/-   ##
==========================================
- Coverage   88.24%   88.24%   -0.01%     
==========================================
  Files         651      651              
  Lines      183930   183865      -65     
  Branches    35850    35823      -27     
==========================================
- Hits       162311   162244      -67     
- Misses      14918    14929      +11     
+ Partials     6701     6692       -9     
Files with missing lines Coverage Δ
lib/internal/webstreams/encoding.js 100.00% <100.00%> (ø)

... and 43 files with indirect coverage changes

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

Looks good other than throwing ERR_INVALID_THIS. ERR_INVALID_ARG_TYPE is likely a better error to be thrown here.

@Nahee-Park
Copy link
Contributor Author

Looks good other than throwing ERR_INVALID_THIS. ERR_INVALID_ARG_TYPE is likely a better error to be thrown here.

Thank you for the suggestion! I’ve updated the code to throw ERR_INVALID_ARG_TYPE instead of ERR_INVALID_THIS as per your recommendation. I appreciate the feedback : )

@Nahee-Park Nahee-Park force-pushed the fix-text-decoder-stream branch from 8020ad2 to cc89cf2 Compare September 29, 2024 10:15
@KhafraDev KhafraDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@KhafraDev KhafraDev added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 29, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55153
✔  Done loading data for nodejs/node/pull/55153
----------------------------------- PR info ------------------------------------
Title      stream: handle undefined chunks correctly in decode stream (#55153)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     Nahee-Park:fix-text-decoder-stream -> nodejs:main
Labels     author ready, needs-ci, web streams
Commits    2
 - stream: handle undefined chunks correctly in decode stream
 - stream: throw ERR_INVALID_ARG_TYPE instead of ERR_INVALID_THIS
Committers 1
 - Nahee-Park <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 28 Sep 2024 16:09:29 GMT
   ✔  Approvals: 2
   ✔  - Mattias Buelens (@MattiasBuelens): https://github.com/nodejs/node/pull/55153#pullrequestreview-2335819701
   ✔  - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/55153#pullrequestreview-2335879581
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-29T15:29:57Z: https://ci.nodejs.org/job/node-test-pull-request/62843/
- Querying data for job/node-test-pull-request/62843/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 55153
From https://github.com/nodejs/node
 * branch                  refs/pull/55153/merge -> FETCH_HEAD
✔  Fetched commits as bbf08c6a1bef..cc89cf23dee8
--------------------------------------------------------------------------------
[main 16d27d5e63] stream: handle undefined chunks correctly in decode stream
 Author: Nahee-Park <[email protected]>
 Date: Sat Sep 28 23:40:33 2024 +0900
 2 files changed, 3 insertions(+), 7 deletions(-)
[main ba985b3895] stream: throw ERR_INVALID_ARG_TYPE instead of ERR_INVALID_THIS
 Author: Nahee-Park <[email protected]>
 Date: Sun Sep 29 19:07:21 2024 +0900
 1 file changed, 2 insertions(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: handle undefined chunks correctly in decode stream

Align TextDecoderStream behavior with WPT requirements by treating
undefined chunks as errors. This change ensures that TextDecoderStream
properly handles unexpected chunk types and throws an error when
receiving undefined input.

This update addresses the failing WPT for decode stream error handling.

PR-URL: #55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>

[detached HEAD dcd4d048ce] stream: handle undefined chunks correctly in decode stream
Author: Nahee-Park <[email protected]>
Date: Sat Sep 28 23:40:33 2024 +0900
2 files changed, 3 insertions(+), 7 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: throw ERR_INVALID_ARG_TYPE instead of ERR_INVALID_THIS

PR-URL: #55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>

[detached HEAD 0413c2cc24] stream: throw ERR_INVALID_ARG_TYPE instead of ERR_INVALID_THIS
Author: Nahee-Park <[email protected]>
Date: Sun Sep 29 19:07:21 2024 +0900
1 file changed, 2 insertions(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/11110112478

@KhafraDev KhafraDev added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 30, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2024
@nodejs-github-bot nodejs-github-bot merged commit 3111ed7 into nodejs:main Sep 30, 2024
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3111ed7

targos pushed a commit that referenced this pull request Oct 4, 2024
Align TextDecoderStream behavior with WPT requirements by treating
undefined chunks as errors. This change ensures that TextDecoderStream
properly handles unexpected chunk types and throws an error when
receiving undefined input.

This update addresses the failing WPT for decode stream error handling.

PR-URL: #55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
Align TextDecoderStream behavior with WPT requirements by treating
undefined chunks as errors. This change ensures that TextDecoderStream
properly handles unexpected chunk types and throws an error when
receiving undefined input.

This update addresses the failing WPT for decode stream error handling.

PR-URL: #55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Align TextDecoderStream behavior with WPT requirements by treating
undefined chunks as errors. This change ensures that TextDecoderStream
properly handles unexpected chunk types and throws an error when
receiving undefined input.

This update addresses the failing WPT for decode stream error handling.

PR-URL: nodejs#55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
@marco-ippolito marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Align TextDecoderStream behavior with WPT requirements by treating
undefined chunks as errors. This change ensures that TextDecoderStream
properly handles unexpected chunk types and throws an error when
receiving undefined input.

This update addresses the failing WPT for decode stream error handling.

PR-URL: nodejs#55153
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants