stream: enable usage of webstreams on compose() by debadree25 · Pull Request #46675 · nodejs/node

debadree25 · 2023-02-15T19:23:18Z

The final one in the series

Refs: #39316

nodejs-github-bot · 2023-02-15T19:23:23Z

Review requested:

@nodejs/streams

debadree25 · 2023-02-15T19:23:35Z

ping @ronag

Refs: nodejs#39316

debadree25 · 2023-02-16T07:26:38Z

lib/internal/streams/compose.js

+    } else {
+      onclose = callback;
+      if (isNodeStream(tail)) {
+        destroyer(tail, err);


Some help is needed here, how could we destroy webstreams here? or should we even?

Can you elaborate on the question? (We can destroy web streams the question is what scenario do you specifically mean)

When the pipeline encounters an error, it would call d.destroy, which in turn would destroy the last stream in the series the tail stream should the same happen for webstreams too I think we could do writableStream.abort() here.

Actually, i am a little confused why destroying the last stream is necessary 😅

This one question remain wdyt @benjamingr ?

debadree25 · 2023-02-16T17:12:19Z

Added tests and doc opening for review

test/parallel/test-webstreams-compose.js

ronag

This is quite complicated. Can you please make this as easy to review as possible, i.e. avoid any unnecessary changes and make the diff as small as possible, e.g. don't rename d to duplex.

debadree25 · 2023-02-17T07:56:25Z

Simplified the diff, and removed the unnecessary functions could you check @ronag

mcollina

lgtm

benjamingr

Looks good, left a few comments that are non-blocking for now but I'd like addressed

lib/internal/streams/compose.js

ronag

I need some time to review this

debadree25 · 2023-02-19T12:16:08Z

No problem!

nodejs-github-bot · 2023-02-26T18:30:11Z

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

mcollina

lgtm

nodejs-github-bot · 2023-02-27T08:50:42Z

Landed in 94e1f8f

debadree25 · 2023-02-27T09:00:25Z

I think we can close #39316 now

implemented:

finished
pipeline
compose
addAbortSignal

Refs: nodejs#39316 PR-URL: nodejs#46675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #39316 PR-URL: #46675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 15, 2023

stream: enable usage of webstreams on compose()

505d884

Refs: nodejs#39316

debadree25 force-pushed the ft/compose-webstreams branch from dce9344 to 505d884 Compare February 15, 2023 19:28

fixup! destroyer

a1cca78

debadree25 commented Feb 16, 2023

View reviewed changes

debadree25 added 2 commits February 16, 2023 22:27

fixup! add tests

bcd122a

fixup! update doc

62f5372

debadree25 changed the title ~~streams: enable usage of webstreams on compose()~~ stream: enable usage of webstreams on compose() Feb 16, 2023

debadree25 marked this pull request as ready for review February 16, 2023 17:12

debadree25 marked this pull request as draft February 16, 2023 17:16

debadree25 added 3 commits February 16, 2023 23:47

fixup! pipeline finishcount

20fd566

fixup! add rejection handle to writer close

4e1d499

fixup! tests some issue

f3c6214

debadree25 marked this pull request as ready for review February 16, 2023 18:23

debadree25 commented Feb 16, 2023

View reviewed changes

test/parallel/test-webstreams-compose.js Outdated Show resolved Hide resolved

fixup! fix unhandled reject, add more tests

8ce94be

ronag reviewed Feb 17, 2023

View reviewed changes

fixup! simplify

838f5ef

mcollina approved these changes Feb 17, 2023

View reviewed changes

benjamingr approved these changes Feb 17, 2023

View reviewed changes

lib/internal/streams/compose.js Show resolved Hide resolved

lib/internal/streams/compose.js Outdated Show resolved Hide resolved

lib/internal/streams/compose.js Outdated Show resolved Hide resolved

debadree25 added 2 commits February 17, 2023 15:44

fixup! unify conditions

183f475

fixup! add update to doc

233115d

This comment was marked as off-topic.

Sign in to view

debadree25 requested a review from ronag February 18, 2023 19:40

github-actions bot mentioned this pull request Feb 19, 2023

CI Reliability 2023-02-19 nodejs/reliability#500

Open

21 tasks

This comment was marked as outdated.

Sign in to view

fixup! update code flow while reading

002eed4

ronag requested changes Feb 19, 2023

View reviewed changes

github-actions bot mentioned this pull request Feb 20, 2023

CI Reliability 2023-02-20 nodejs/reliability#501

Open

24 tasks

jasnell approved these changes Feb 20, 2023

View reviewed changes

github-actions bot mentioned this pull request Feb 21, 2023

CI Reliability 2023-02-21 nodejs/reliability#502

Open

26 tasks

debadree25 added stream Issues and PRs related to the stream subsystem. web streams request-ci Add this label to start a Jenkins CI on a PR. labels Feb 22, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2023

ronag approved these changes Feb 27, 2023

View reviewed changes

mcollina approved these changes Feb 27, 2023

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Feb 27, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2023

nodejs-github-bot merged commit 94e1f8f into nodejs:main Feb 27, 2023

debadree25 mentioned this pull request Feb 27, 2023

[v18.x backport] stream: streams and webstreams interop related changes #46314

Closed

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

Uh oh!

Conversation

debadree25 commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 15, 2023

Uh oh!

debadree25 commented Feb 15, 2023

Uh oh!

debadree25 Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Feb 16, 2023

Uh oh!

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Feb 17, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as outdated.

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Feb 19, 2023

Uh oh!

nodejs-github-bot commented Feb 26, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 27, 2023

Uh oh!

debadree25 commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

debadree25 commented Feb 15, 2023 •

edited

Loading

debadree25 Feb 16, 2023 •

edited

Loading