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

stream: compose #39029

Closed
wants to merge 8 commits into from
Closed

stream: compose #39029

wants to merge 8 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 14, 2021

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
Copy link
Member Author

ronag commented Jun 14, 2021

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

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

targos commented Jun 14, 2021

@nodejs/streams

@vweevers
Copy link
Contributor

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

@mscdex
Copy link
Contributor

mscdex commented Jun 14, 2021

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
Copy link
Contributor

I'm not familiar with the use cases for this

#32020

@ronag
Copy link
Member Author

ronag commented Jun 14, 2021

Does this really need to exist in node core?

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

@vweevers
Copy link
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
Copy link
Member Author

ronag commented Jun 14, 2021

Rxjs does 1. So there is some precedence.

@nodejs-github-bot
Copy link
Collaborator

ronag added a commit that referenced this pull request Jul 19, 2021
Refs: #32020

PR-URL: #39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ronag
Copy link
Member Author

ronag commented Jul 19, 2021

Landed in e579acb

@targos
Copy link
Member

targos commented Jul 21, 2021

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

@ronag ronag added the notable-change PRs with changes that should be highlighted in changelogs. label Jul 28, 2021
ronag added a commit to nxtedition/node that referenced this pull request Jul 28, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL:
ronag added a commit to nxtedition/node that referenced this pull request Jul 28, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
@ronag
Copy link
Member Author

ronag commented Jul 28, 2021

#39563

ronag added a commit to nxtedition/node that referenced this pull request Jul 29, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Jul 29, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Aug 1, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Aug 1, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Aug 1, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
Refs: nodejs#32020

PR-URL: nodejs#39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: nodejs#39563
targos pushed a commit that referenced this pull request Sep 6, 2021
Refs: #32020

PR-URL: #39029
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-PR-URL: #39563
targos added a commit that referenced this pull request Sep 6, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851
deps:
  * (SEMVER-MINOR) add corepack (Maël Nison) #39608
  * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
stream:
  * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029

PR-URL: #40011
@targos targos mentioned this pull request Sep 6, 2021
targos added a commit that referenced this pull request Sep 6, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851
deps:
  * (SEMVER-MINOR) add corepack (Maël Nison) #39608
  * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
stream:
  * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029

PR-URL: #40011
targos added a commit that referenced this pull request Sep 7, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851
deps:
  * (SEMVER-MINOR) add corepack (Maël Nison) #39608
  * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
stream:
  * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029

PR-URL: #40011
codebytere added a commit to electron/electron that referenced this pull request Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

10 participants