GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: allow byob reader for 'blob.stream()' by debadree25 · Pull Request #49713 · nodejs/node

Conversation

@debadree25

@debadree25 debadree25 commented Sep 19, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fixes: #47993

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@debadree25
lib: allow byob reader for 'blob.stream()'
50ab557 
Fixes: nodejs#47993
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 19, 2023
@debadree25 debadree25 mentioned this pull request Sep 19, 2023
byob reader support for Blob.stream() #47993
Closed
@debadree25
fixup! fix structured cloning wpt and add comments
23cf23f
@debadree25
debadree25 marked this pull request as ready for review September 19, 2023 10:34
debadree25
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 debadree25 Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@debadree25

debadree25 commented Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

cc @nodejs/whatwg-stream
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25
fixup! lint
7e446f1
@debadree25 debadree25 added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. web streams Issues and PRs related to the Web Streams API. labels Sep 19, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 19, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Sep 20, 2023
CI Reliability 2023-09-20 nodejs/reliability#671
Open
CI Reliability 2023-09-21 nodejs/reliability#672
Open
@nodejs-github-bot

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Sep 21, 2023
View reviewed changes
@debadree25 debadree25 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25 debadree25 added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Sep 21, 2023
mcollina
mcollina approved these changes Sep 21, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
debadree25 reacted with heart emoji debadree25 reacted with rocket emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction
@github-actions github-actions Bot mentioned this pull request Sep 22, 2023
CI Reliability 2023-09-22 nodejs/reliability#673
Open
17 tasks
@debadree25 debadree25 added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 22, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 23a3410 into nodejs:main Sep 22, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 22, 2023

Copy link
Copy Markdown
Collaborator

Landed in 23a3410
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 24, 2023
CI Reliability 2023-09-24 nodejs/reliability#675
Open
20 tasks
@debadree25
debadree25 deleted the ft/blob-byob branch September 24, 2023 17:31
@debadree25 debadree25 mentioned this pull request Sep 26, 2023
wpt/test-structured-clone is flaky #49852
Closed
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@debadree25 @ruyadorno
lib: allow byob reader for 'blob.stream()'
1287d5b 
Fixes: #47993
PR-URL: #49713
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 28, 2023
v20.7.1 proposal #49917
Closed
@ruyadorno ruyadorno mentioned this pull request Sep 28, 2023
v20.8.0 proposal #49932
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@benjamingr benjamingr benjamingr approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

byob reader support for Blob.stream()

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@debadree25 @nodejs-github-bot @mcollina @benjamingr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.