-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add compose operator #44937
stream: add compose operator #44937
Conversation
Review requested:
|
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 oldcompose
api? - documents is needed
Thanks for reviewing
I don't think so, it's still a valuable api
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 😀 |
@ronag @benjamingr @jasnell should I keep developing it? I don't wanna waste my time in case we decide not to do it 😊 |
Hey sorry, just getting on top of things and will review. |
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
Added validation, aside from the docs I think this PR is ready It would be great if someone could help me with the docs |
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
doc/api/stream.md
Outdated
|
||
|
||
|
||
See [`stream.compose`][] for more information. |
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?
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 👍🏻
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 :)
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... |
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)https://github.com/nodejs/node/actions/runs/3361645437 |
Landed in ddb3ae7 |
PR-URL: #44937 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
PR-URL: nodejs#44937 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
PR-URL: #44937 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
PR-URL: #44937 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
PR-URL: #44937 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
PR-URL: #44937 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
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 theaddAbortSignal
but we won't pass the stream the signal as the rest of the operatorsCc @benjamingr