Skip to content

fs: rename FileHandle#readableWebStream to stream #40016

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.

Sign up for GitHub

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
Closed
wants to merge 1 commit into from
Closed

fs: rename FileHandle#readableWebStream to stream #40016

wants to merge 1 commit into from

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 6, 2021

It makes the method more aligned with the web standard
Blob.prototype.stream.

Now is a good time as ever to discuss renaming the method as it hasn't landed on any release line yet.

Refs: https://w3c.github.io/FileAPI/#dom-blob-stream

@aduh95
fs: rename FileHandle readableWebStream method to stream
9dcad9a 
It makes the method more aligned with the web standard
`Blob.prototype.stream`.

Refs: https://w3c.github.io/FileAPI/#dom-blob-stream
@aduh95 aduh95 added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 6, 2021
@aduh95 aduh95 requested a review from jasnell September 6, 2021 15:20
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 6, 2021
@aduh95 aduh95 added experimental Issues and PRs related to experimental features. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 6, 2021
@jasnell
Copy link
Member

jasnell commented Sep 6, 2021

Using readableWebStream() was intentional in response to feedback from @ronag about possible/likely confusion between using ReadableStream and stream.Readable.

@ronag
Copy link
Member

ronag commented Sep 6, 2021

I’m fine with changing. Just wish we could have some form of guideline with naming that is consistent.

@aduh95 aduh95 mentioned this pull request Sep 6, 2021
Closed
@ronag
Copy link
Member

ronag commented Sep 7, 2021 •
edited
Loading

What about just implementing https://fetch.spec.whatwg.org/#body-mixin?

i.e. body getter returns a stream.

Ah then there is https://w3c.github.io/FileAPI/#stream-method-algo

Webstandard is messy :/

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 19, 2021

Node.js 17.0.0 is out with FileHandle#readableWebStream. Closing.

@aduh95 aduh95 closed this Oct 19, 2021
@aduh95 aduh95 deleted the rename-readableWebStream-to-stream branch October 19, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell Awaiting requested review from jasnell

Assignees

No one assigned

Labels

experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aduh95 @jasnell @ronag @nodejs-github-bot