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

stream: compose by ronag · Pull Request #39029 · nodejs/node · GitHub

/ node Public

stream: compose - #39029

Closed
ronag wants to merge 8 commits into
nodejs:masterfrom
nxtedition:stream-pipe
ConversationCommits (8)ChecksFiles changed
Closed

stream: compose#39029
ronag wants to merge 8 commits into
nodejs:masterfrom
nxtedition:stream-pipe

Conversation

ronag commented Jun 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

pipe is similar to pipeline however it supports stream composition, i.e.

const transform3 = stream.pipe(transform1, transform2)
// transform3 is a writable

stream.pipeline(source, transform3, sink, (err) => console.log('err'))

Similar to how rx js provides a top level pipe(...observables) method.

ronag commented Jun 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nodejs/streams Looking for some initial feedback before starting work on tests.

github-actions Bot added the needs-ci PRs that need a full CI run. label Jun 14, 2021
ronag added the stream Issues and PRs related to the stream subsystem. label Jun 14, 2021
ronag force-pushed the stream-pipe branch 19 times, most recently from 2e23587 to 46ddf18 Compare June 14, 2021 14:59

targos commented Jun 14, 2021

Copy link
Copy Markdown
Member

@nodejs/streams

vweevers commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

@ronag Knowing that stream.pipeline() is the equivalent of pump, can we say that stream.pipe() is the equivalent of pumpify?

mscdex commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

I'm not familiar with the use cases for this, but I think it can be a bit confusing because of the function name (and where it's being exported) and because we already have the pre-existing pipe() and pipeline(). Does this really need to exist in node core?

vweevers commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

I'm not familiar with the use cases for this

#32020

ronag commented Jun 14, 2021

Copy link
Copy Markdown
Member Author

Does this really need to exist in node core?

There is #32020 and also pumpify which is not entirely correct.

vweevers commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

I think it can be a bit confusing because of the function name [and] the pre-existing pipe()

Some options:

  1. No bikeshedding, risking some confusion. Call it pipe() because it's a fitting term. The way I see it, multiple open-ended "pipes" can be combined to form a closed-ended "pipeline". When context requires it, use additional words to differentiate between stream.pipe and stream.prototype.pipe. Same as events.once versus events.prototype.once.
  2. Make up a new distinct term, which could take more effort to explain. Ex: segment(), compose().
  3. Make it descriptive at the cost of verbosity. Ex: duplexPipeline(), duplexFrom(), writablePipeline().

ronag commented Jun 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Rxjs does 1. So there is some precedence.

MattiasBuelens commented Jul 8, 2021

Copy link
Copy Markdown
Contributor

@ronag Sure! 🙂 If it matters for scheduling purposes: I'm based in Europe (currently CEST).

ronag commented Jul 8, 2021

Copy link
Copy Markdown
Member Author

I'm CEST as well. I can be quite flexible next week. What about @benjamingr?

jasnell reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/streams/compose.js Outdated
jasnell mentioned this pull request Jul 9, 2021
Closed
mcollina approved these changes Jul 12, 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

nodejs-github-bot commented Jul 12, 2021

Copy link
Copy Markdown
Collaborator

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

ronag commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

This has sufficient approvals but I think @benjamingr would like to further discuss this before landing? @benjamingr would you mind doing a "request for changes" so we don't accidentally land this before that?

nodejs-github-bot commented Jul 12, 2021

Copy link
Copy Markdown
Collaborator

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

ronag commented Jul 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nodejs/streams @mcollina I've disabled async function support. I believe in this way this can land without any pending items. I'll open a separate PR we can discuss in regards to async fn/gen API.

This way we can continue with e.g. web stream support

Please 👍 so I know whether I can land this.

nodejs-github-bot commented Jul 12, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 12, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 12, 2021

Copy link
Copy Markdown
Collaborator

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

targos reviewed Jul 12, 2021
View reviewed changes
Comment thread test/parallel/test-bootstrap-modules.js Outdated
targos approved these changes Jul 12, 2021
View reviewed changes
Comment thread lib/stream.js Outdated
Comment thread lib/stream.js
Comment thread lib/internal/streams/compose.js Outdated
Comment thread lib/internal/streams/compose.js Outdated
ronag and others added 2 commits July 13, 2021 10:06
Apply suggestions from code review
87302f0 
Co-authored-by: Michaël Zasso <targos@protonmail.com>
fixup
dc80a26

nodejs-github-bot commented Jul 14, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 14, 2021

Copy link
Copy Markdown
Collaborator

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

ronag commented Jul 19, 2021

Copy link
Copy Markdown
Member Author

Landed in e579acb

ronag mentioned this pull request Jul 19, 2021
Closed

targos commented Jul 21, 2021

Copy link
Copy Markdown
Member

Like #39134 (comment), this needs a backport to land on v16.x because it depends on the semver-major #39294

targos mentioned this pull request Jul 26, 2021
Closed

ronag commented Jul 28, 2021

Copy link
Copy Markdown
Member Author

#39563

targos mentioned this pull request Sep 6, 2021
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

MattiasBuelens MattiasBuelens left review comments
mcollina mcollina approved these changes
lpinca lpinca approved these changes
benjamingr Awaiting requested review from benjamingr
jasnell Awaiting requested review from jasnell
+2 more reviewers
targos targos approved these changes
vweevers vweevers left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

notable-change PRs with changes that should be highlighted in changelogs. 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.

10 participants

Back | FazBrowse Home | New Git URL