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: enable usage of webstreams on compose() #46675

Merged
merged 12 commits into from
Feb 27, 2023

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Feb 15, 2023

The final one in the series

Refs: #39316

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 15, 2023
@debadree25
Copy link
Member Author

ping @ronag

} else {
onclose = callback;
if (isNodeStream(tail)) {
destroyer(tail, err);
Copy link
Member Author

@debadree25 debadree25 Feb 16, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

This one question remain wdyt @benjamingr ?

@debadree25 debadree25 changed the title streams: enable usage of webstreams on compose() stream: enable usage of webstreams on compose() Feb 16, 2023
@debadree25
Copy link
Member Author

Added tests and doc opening for review

@debadree25 debadree25 marked this pull request as ready for review February 16, 2023 17:12
@debadree25 debadree25 marked this pull request as draft February 16, 2023 17:16
@debadree25 debadree25 marked this pull request as ready for review February 16, 2023 18:23
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

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

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

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

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
@nodejs-github-bot

This comment was marked as off-topic.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I need some time to review this

@debadree25
Copy link
Member Author

No problem!

@debadree25 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina 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 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 nodejs-github-bot merged commit 94e1f8f into nodejs:main Feb 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 94e1f8f

@debadree25
Copy link
Member Author

I think we can close #39316 now

implemented:

  • finished
  • pipeline
  • compose
  • addAbortSignal

debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. stream Issues and PRs related to the stream subsystem. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants