GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: add compose operator by rluvaton · Pull Request #44937 · nodejs/node

Conversation

@rluvaton

@rluvaton rluvaton commented Oct 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Start adding compose function #43664

I've started implementing from scratch and then realized that I don't need to, Using the compose function is enough.

The only problem I have is with the abort signal as the static compose doesn't seem to support it so I used the addAbortSignal but we won't pass the stream the signal as the rest of the operators

Cc @benjamingr

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pimterry reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
rluvaton added 2 commits October 9, 2022 13:43
@rluvaton
stream: add compose operator
076bbe5
@rluvaton
stream: rename import
da9255f
@nodejs-github-bot

nodejs-github-bot commented Oct 9, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 9, 2022
@rluvaton
stream: add abort signal support
273b0b6
rluvaton
rluvaton commented Oct 9, 2022
View reviewed changes
Comment thread lib/internal/streams/operators.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

rluvaton
rluvaton commented Oct 9, 2022
View reviewed changes
Comment thread lib/internal/streams/operators.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton

rluvaton commented Oct 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Cc @ronag

#43664 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xtx1130
xtx1130 reviewed Oct 18, 2022
View reviewed changes

@xtx1130 xtx1130 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

  • If you change compose to chain use, whether we should hide the old compose api?
  • documents is needed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rluvaton

rluvaton commented Oct 18, 2022

Copy link
Copy Markdown
Member Author

Thanks for reviewing

  • If you change compose to chain use, whether we should hide the old compose api?

I don't think so, it's still a valuable api

  • documents is needed

I did not add a documentation yet cause @ronag said that this may not be merged, so I'll wait for merge approval and then I'll write the docs 😀

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton

rluvaton commented Oct 18, 2022

Copy link
Copy Markdown
Member Author

@ronag @benjamingr @jasnell should I keep developing it? I don't wanna waste my time in case we decide not to do it 😊

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Oct 26, 2022

Copy link
Copy Markdown
Member

Hey sorry, just getting on top of things and will review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Oct 26, 2022
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm in favor of it, needs docs before it can land

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
rluvaton added 2 commits October 26, 2022 22:02
@rluvaton
stream: add validation and refactor tests
2e4898e
@rluvaton
stream: fix lint and add started adding docs
fed2ac3
@rluvaton

rluvaton commented Oct 26, 2022

Copy link
Copy Markdown
Member Author

Added validation, aside from the docs I think this PR is ready

It would be great if someone could help me with the docs

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton
rluvaton commented Oct 26, 2022
View reviewed changes
Comment thread test/parallel/test-stream-compose.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Oct 27, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 27, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 27, 2022
benjamingr
benjamingr reviewed Oct 27, 2022
View reviewed changes
Comment thread doc/api/stream.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Oct 27, 2022
View reviewed changes
Comment thread doc/api/stream.md Outdated



See [`stream.compose`][] for more information.

@benjamingr benjamingr Oct 27, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Add an example please?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@rluvaton rluvaton Oct 27, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Added 👍🏻

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
benjamingr
benjamingr requested changes Oct 27, 2022
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good sans a few changes - blocking so this doesn't land before then :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rluvaton
stream: added an example in the streams docs
da244b9
@rluvaton
rluvaton requested a review from benjamingr October 27, 2022 22:40
ronag
ronag approved these changes Oct 27, 2022
View reviewed changes
@rluvaton

rluvaton commented Oct 27, 2022

Copy link
Copy Markdown
Member Author

What to do about the failing lint? it fails cause the line is too long, but the line is long because of the link which is not displayed...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr approved these changes Oct 28, 2022
View reviewed changes
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 28, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 28, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 28, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton
stream: fix lint error
7e55077
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 30, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 30, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 30, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 31, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr approved these changes Oct 31, 2022
View reviewed changes
@benjamingr benjamingr added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 31, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 31, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 31, 2022

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/44937
✔  Done loading data for nodejs/node/pull/44937
----------------------------------- PR info ------------------------------------
Title      stream: add compose operator (#44937)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     rluvaton:feat/add-compose-stream-operator -> nodejs:main
Labels     needs-ci
Commits    7
 - stream: add compose operator
 - stream: rename import
 - stream: add abort signal support
 - stream: add validation and refactor tests
 - stream: fix lint and add started adding docs
 - stream: added an example in the streams docs
 - stream: fix lint error
Committers 1
 - Raz Luvaton <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/44937
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Robert Nagy 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44937
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Robert Nagy 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 09 Oct 2022 10:50:07 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/44937#pullrequestreview-1158053238
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/44937#pullrequestreview-1161868722
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/44937#pullrequestreview-1159123210
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-31T11:41:05Z: https://ci.nodejs.org/job/node-test-pull-request/47589/
- Querying data for job/node-test-pull-request/47589/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 44937
From https://github.com/nodejs/node
 * branch                  refs/pull/44937/merge -> FETCH_HEAD
✔  Fetched commits as ffa2e964e882..7e55077b63a6
--------------------------------------------------------------------------------
[main 91eaa5aa1f] stream: add compose operator
 Author: Raz Luvaton <[email protected]>
 Date: Sun Oct 9 13:43:49 2022 +0300
 2 files changed, 98 insertions(+)
 create mode 100644 test/parallel/test-stream-compose-operator.js
[main 18e34bf99c] stream: rename import
 Author: Raz Luvaton <[email protected]>
 Date: Sun Oct 9 13:46:14 2022 +0300
 1 file changed, 2 insertions(+), 2 deletions(-)
[main 12370446d3] stream: add abort signal support
 Author: Raz Luvaton <[email protected]>
 Date: Sun Oct 9 16:04:37 2022 +0300
 2 files changed, 55 insertions(+), 11 deletions(-)
[main 52e55a1bb8] stream: add validation and refactor tests
 Author: Raz Luvaton <[email protected]>
 Date: Wed Oct 26 22:02:02 2022 +0300
 3 files changed, 30 insertions(+), 16 deletions(-)
Auto-merging doc/api/stream.md
[main b9d363ceb1] stream: fix lint and add started adding docs
 Author: Raz Luvaton <[email protected]>
 Date: Wed Oct 26 22:21:10 2022 +0300
 3 files changed, 24 insertions(+), 5 deletions(-)
Auto-merging doc/api/stream.md
[main 261e4c014b] stream: added an example in the streams docs
 Author: Raz Luvaton <[email protected]>
 Date: Fri Oct 28 01:39:24 2022 +0300
 1 file changed, 20 insertions(+), 2 deletions(-)
Auto-merging doc/api/stream.md
[main bcd6bd6930] stream: fix lint error
 Author: Raz Luvaton <[email protected]>
 Date: Sat Oct 29 23:09:44 2022 +0300
 1 file changed, 4 insertions(+), 2 deletions(-)
   ✔  Patches applied
There are 7 commits in the PR. Attempting autorebase.
Rebasing (2/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: add compose operator

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD 115742ad0f] stream: add compose operator
Author: Raz Luvaton [email protected]
Date: Sun Oct 9 13:43:49 2022 +0300
2 files changed, 98 insertions(+)
create mode 100644 test/parallel/test-stream-compose-operator.js
Rebasing (3/14)
Rebasing (4/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: rename import

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD ec8d16b956] stream: rename import
Author: Raz Luvaton [email protected]
Date: Sun Oct 9 13:46:14 2022 +0300
1 file changed, 2 insertions(+), 2 deletions(-)
Rebasing (5/14)
Rebasing (6/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: add abort signal support

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD 44d7c6d427] stream: add abort signal support
Author: Raz Luvaton [email protected]
Date: Sun Oct 9 16:04:37 2022 +0300
2 files changed, 55 insertions(+), 11 deletions(-)
Rebasing (7/14)
Rebasing (8/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: add validation and refactor tests

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD ee4bffce62] stream: add validation and refactor tests
Author: Raz Luvaton [email protected]
Date: Wed Oct 26 22:02:02 2022 +0300
3 files changed, 30 insertions(+), 16 deletions(-)
Rebasing (9/14)
Rebasing (10/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: fix lint and add started adding docs

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD 1f23e92eee] stream: fix lint and add started adding docs
Author: Raz Luvaton [email protected]
Date: Wed Oct 26 22:21:10 2022 +0300
3 files changed, 24 insertions(+), 5 deletions(-)
Rebasing (11/14)
Rebasing (12/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: added an example in the streams docs

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD e08fe74562] stream: added an example in the streams docs
Author: Raz Luvaton [email protected]
Date: Fri Oct 28 01:39:24 2022 +0300
1 file changed, 20 insertions(+), 2 deletions(-)
Rebasing (13/14)
Rebasing (14/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: fix lint error

PR-URL: #44937
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]

[detached HEAD 581d9ddecc] stream: fix lint error
Author: Raz Luvaton [email protected]
Date: Sat Oct 29 23:09:44 2022 +0300
1 file changed, 4 insertions(+), 2 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/3361645437

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr benjamingr added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Oct 31, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 31, 2022
@nodejs-github-bot
nodejs-github-bot merged commit ddb3ae7 into nodejs:main Oct 31, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 31, 2022

Copy link
Copy Markdown
Collaborator

Landed in ddb3ae7

benjamingr and rluvaton reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Nov 1, 2022
CI Reliability 2022-11-01 nodejs/reliability#417
Open
17 tasks
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@rluvaton @RafaelGSS
stream: add compose operator
dfd75c1
PR-URL: #44937
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
v19.1.0 proposal #45269
Merged
@github-actions github-actions Bot mentioned this pull request Nov 2, 2022
CI Reliability 2022-11-02 nodejs/reliability#418
Open
15 tasks
@rluvaton
rluvaton deleted the feat/add-compose-stream-operator branch November 4, 2022 09:29
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@rluvaton @RafaelGSS
stream: add compose operator
ccb1c1e
PR-URL: #44937
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@rluvaton @danielleadams
stream: add compose operator
3559ac1
PR-URL: #44937
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@rluvaton @danielleadams
stream: add compose operator
5f7d2b5
PR-URL: #44937
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@Renegade334 Renegade334 mentioned this pull request Oct 1, 2024
stream: readable.compose fails to return a Duplex #55203
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@benjamingr benjamingr benjamingr approved these changes
@ronag ronag ronag approved these changes
+1 more reviewer
@xtx1130 xtx1130 xtx1130 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@rluvaton @nodejs-github-bot @benjamingr @mcollina @ronag @xtx1130
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.