X Tutup
The Wayback Machine - https://web.archive.org/web/20230425132749/https://github.com/nodejs/node/pull/46933
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

Jump to bottom

fs: implement byob mode for readableWebStream() #46933

Merged
merged 3 commits into from Apr 10, 2023
Merged

fs: implement byob mode for readableWebStream() #46933

merged 3 commits into from Apr 10, 2023

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Mar 3, 2023 •
edited

This is an initial draft trying to implement allowing usage of 'byob' mode for readable web streams created from file handles.

Would like some feedback before I start working on tests :-)

Have added tests and doc updates,
the implementation is similar to how the implementation is here https://nodejs.org/api/webstreams.html#class-readablestreambyobreader

Fixes: #45853

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 3, 2023
@debadree25 debadree25 requested a review from jasnell March 3, 2023 11:09
@debadree25

This comment was marked as outdated.

Sign in to view
@debadree25 debadree25 closed this Mar 4, 2023
@debadree25 debadree25 reopened this Mar 4, 2023
@debadree25 debadree25 removed the request for review from jasnell March 4, 2023 19:08
@debadree25 debadree25 marked this pull request as ready for review March 4, 2023 19:10
@debadree25 debadree25 requested a review from jasnell March 4, 2023 19:10
@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 5, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@debadree25 debadree25 added the review wanted PRs that need reviews. label Mar 6, 2023
@debadree25
Copy link
Member Author

also cc'ing @nodejs/whatwg-stream

@debadree25
Copy link
Member Author

Ping @nodejs/whatwg-stream , @nodejs/streams ?

@nodejs-github-bot
Copy link
Contributor

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

@debadree25
Copy link
Member Author

no one to review? 😕
any direction on whether this direction is appropriate or not would also be helpful!

@benjamingr
Copy link
Member

no one to review? 😕 any direction on whether this direction is appropriate or not would also be helpful!

I suspect many people (at least me) are in spring break holidays Mar/Apr (easter/passover/holi/nowruz/songkram etc)

Also, web streams typically don't get a ton of love (that makes your PRs more welcome not less! And it's good that you're tackling them) but most of the node/streams people (at least matteo, ronag and myself) aren't web-stream experts.

@debadree25
Copy link
Member Author

Hey @benjamingr yeah i also thought people are probably busy rn! no problem at all I will leave the PR open for the time being no hurries 😄😄!

benjamingr
benjamingr approved these changes Apr 9, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Ah this just implements the ByoB stuff for fs on top of web streams - that should be fine and much smaller to review

@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Contributor

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

@debadree25
Copy link
Member Author

CI seems to be happy! just need one more review I guess!

@debadree25 debadree25 added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 10, 2023
mcollina
mcollina approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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 needs-ci PRs that need a full CI run. labels Apr 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 10, 2023
@nodejs-github-bot nodejs-github-bot merged commit a9c3d92 into nodejs:main Apr 10, 2023
68 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in a9c3d92

@RafaelGSS RafaelGSS mentioned this pull request Apr 11, 2023
Merged
This was referenced Apr 12, 2023
Open
Open
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
@debadree25 @RafaelGSS
fs: implement byob mode for readableWebStream()
d4b440b 
Fixes: #45853
PR-URL: #46933
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschanaz saschanaz saschanaz left review comments

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell

@ronag ronag

Assignees
No one assigned
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. fs Issues and PRs related to the fs subsystem / file system. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Byte stream support for FileHandle::readableWebStream
5 participants
@debadree25 @nodejs-github-bot @benjamingr @mcollina @saschanaz
X Tutup