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

stream: utility consumers for web and node.js streams by jasnell · Pull Request #39594 · nodejs/node · GitHub

/ node Public

stream: utility consumers for web and node.js streams - #39594

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:web-streams-consumers
ConversationCommits (1)ChecksFiles changed
Closed

stream: utility consumers for web and node.js streams#39594
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:web-streams-consumers

Conversation

jasnell commented Jul 30, 2021

Copy link
Copy Markdown
Member

@mcollina @ronag ... here's an alternative approach to the Body mixin stuff. Rather than providing the Body mixin directly, these introduce utility functions that can be used by the ecosystem to provide those basic methods, at least in part.

For a very rudimentary example...

const {
  arrayBuffer,
  blob,
  json,
  text,
} = require('stream/consumers');

const kStream = Symbol('kStream');

const MyObjectWithBodyMixin {

  arrayBuffer() { return arrayBuffer(this[kStream]); }

  blob() { return blob(this[kStream]); }

  json() { return json(this[kStream]); }

  text() { return text(this[kStream]); }

}

These work with ReadableStream, stream.Readable, and async interables.

There's likely a bit more error handling that could be added but I wanted to at least open the PR to give a basic idea.

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 30, 2021

Mesteery commented Jul 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Could a buffer(stream) function also be added?

VoltrexKeyva reviewed Jul 30, 2021
View reviewed changes
Comment thread doc/api/webstreams.md Outdated
ronag reviewed Jul 31, 2021
View reviewed changes

ronag left a comment •
edited
Loading

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. I think error and type checking is done indirectly.

ronag reviewed Jul 31, 2021
View reviewed changes
Comment thread lib/stream/consumers.js Outdated
mcollina approved these changes Jul 31, 2021
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

targos added stream Issues and PRs related to the stream subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 1, 2021
Mesteery mentioned this pull request Aug 2, 2021
Closed
jasnell force-pushed the web-streams-consumers branch 3 times, most recently from 344dba7 to d0efbc0 Compare August 3, 2021 20:47

This comment has been minimized.

Sign in to view

targos commented Aug 4, 2021

Copy link
Copy Markdown
Member

What happens if the functions are called on object streams?

ronag reviewed Aug 4, 2021
View reviewed changes
Comment thread lib/stream/consumers.js Outdated
ronag reviewed Aug 4, 2021
View reviewed changes
Comment thread lib/stream/consumers.js Outdated

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Aug 5, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

jasnell commented Aug 6, 2021

Copy link
Copy Markdown
Member Author

@targos:

What happens if the functions are called on object streams?

For blob(), buffer() and arrayBuffer(), the object is coerced using toString() rules. For text() and json() the promises reject because the objects cannot be decoded as utf8 byte sequences. I've extended the tests accordingly.

stream: utility consumers for web and node.js streams
ac912f5 
Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell force-pushed the web-streams-consumers branch from d0efbc0 to ac912f5 Compare August 6, 2021 03:21
jasnell requested a review from ronag August 6, 2021 14:48
jasnell added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Aug 6, 2021

jasnell commented Aug 6, 2021

Copy link
Copy Markdown
Member Author

Landed in c524107

jasnell closed this Aug 6, 2021
jasnell added a commit that referenced this pull request Aug 6, 2021
stream: utility consumers for web and node.js streams
c524107 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39594
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ronag mentioned this pull request Aug 9, 2021
Closed
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
stream: utility consumers for web and node.js streams
f57a0e4 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39594
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams mentioned this pull request Aug 16, 2021
Merged
carlansley mentioned this pull request Aug 19, 2021
Closed

jimmywarting commented Aug 20, 2021 •
edited
Loading

Copy link
Copy Markdown

I wished that buffer() wasn't added
If one would want to make a user-land package out of consumers then you are depending on Buffer that don't fit well into Deno or Browsers.
If someone would have really wanted a Buffer then they could just have done: arrayBuffer(stream).then(Buffer.from)

I'm a bit biased towards Buffer in general cuz it isn't cross env friendly. and it's bloated with stuff TextEncoder and DataView is suppose to solve for you when working with typed arrays

jimmywarting commented Aug 20, 2021

Copy link
Copy Markdown

Would it be optimizable if arrayBuffer() had a totalLength option too?

There would not be pkg like this otherwise that don't need to take up twice the size when it's time to concatinate:
https://github.com/feross/stream-with-known-length-to-buffer

jasnell mentioned this pull request Aug 25, 2021
Merged
Ayase-252 mentioned this pull request Aug 26, 2021
Closed

targos commented Oct 9, 2021

Copy link
Copy Markdown
Member

Like #39134, this needs a volunteer to backport to v16.x-staging.

Mesteery commented Oct 9, 2021

Copy link
Copy Markdown
Contributor

I'm willing to take care of it.

Mesteery commented Oct 9, 2021

Copy link
Copy Markdown
Contributor

Isn't it already good?
https://github.com/nodejs/node/blob/v16.x-staging/lib/stream/consumers.js
@targos

targos commented Oct 9, 2021

Copy link
Copy Markdown
Member

@Mesteery you're right, sorry. It looks like the backport-requested label was added by mistake

bakkot mentioned this pull request Aug 19, 2025
Closed
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
ronag Awaiting requested review from ronag
+1 more reviewer
VoltrexKeyva VoltrexKeyva left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL