FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: allow byob reader for 'blob.stream()' by debadree25 · Pull Request #49713 · nodejs/node · GitHub

/ node Public

lib: allow byob reader for 'blob.stream()' - #49713

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
debadree25:ft/blob-byob
Sep 22, 2023
ConversationCommits (3)ChecksFiles changed
Merged

lib: allow byob reader for 'blob.stream()'#49713
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
debadree25:ft/blob-byob

Conversation

debadree25 commented Sep 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #47993

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 19, 2023
debadree25 mentioned this pull request Sep 19, 2023
Closed
debadree25 marked this pull request as ready for review September 19, 2023 10:34
debadree25 commented Sep 19, 2023
View reviewed changes
Comment thread lib/internal/blob.js
// hit the flow rate of the stream (c.desiredSize).
queueMicrotask(() => {
if (c.desiredSize <= 0) {
if (c.desiredSize < 0) {

debadree25 Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would draw attention of reviewers to this specific update it seems when we turn this into a ReadableByteStream the high water mark for bytestreams is 0 and for normal default streams is 1 hence we have to remove the equality here
see:

node/lib/internal/webstreams/readablestream.js

Lines 282 to 298 in 2ccfb23

if (`${type}` === 'bytes') {
if (size !== undefined)
throw new ERR_INVALID_ARG_VALUE.RangeError('strategy.size', size);
setupReadableByteStreamControllerFromSource(
this,
source,
extractHighWaterMark(highWaterMark, 0));
} else {
if (type !== undefined)
throw new ERR_INVALID_ARG_VALUE('source.type', type);
setupReadableStreamDefaultControllerFromSource(
this,
source,
extractHighWaterMark(highWaterMark, 1),
extractSizeAlgorithm(size));
}
}

also each byte is considered to be an element in the case of byte streams and hence the desiredSize can go below 0, I nonetheless asserted that the memory isnt overflowing in the existing tests but do take a look at them too if they look correct!

Thank You!

debadree25 commented Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

cc @nodejs/whatwg-stream

debadree25 added request-ci Add this label to start a Jenkins CI on a PR. web streams labels Sep 19, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 19, 2023

nodejs-github-bot commented Sep 19, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54059/

This was referenced Sep 20, 2023
Open
Open

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54110/

debadree25 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 21, 2023

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54112/

debadree25 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 21, 2023
mcollina approved these changes Sep 21, 2023
View reviewed changes

mcollina 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

github-actions Bot mentioned this pull request Sep 22, 2023
Open
17 tasks
debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023
nodejs-github-bot merged commit 23a3410 into nodejs:main Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

Copy link
Copy Markdown
Collaborator

Landed in 23a3410

github-actions Bot mentioned this pull request Sep 24, 2023
Open
20 tasks
debadree25 deleted the ft/blob-byob branch September 24, 2023 17:31
debadree25 mentioned this pull request Sep 26, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
lib: allow byob reader for 'blob.stream()'
1287d5b 
Fixes: #47993
PR-URL: #49713
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno mentioned this pull request Sep 28, 2023
Closed
ruyadorno mentioned this pull request Sep 28, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
benjamingr benjamingr approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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. needs-ci PRs that need a full CI run. web streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

byob reader support for Blob.stream()

4 participants

Back | FazBrowse Home | New Git URL