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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Jan 23, 2023 •
edited
Loading

Backports the following PRs:
#46190
#46273
#46307
#46312
#46315
#46403
#46600
#46675

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. web streams labels Jan 23, 2023
@debadree25
Copy link
Member Author

cc @aduh95

@aduh95
Copy link
Contributor

aduh95 commented Jan 23, 2023

We need to wait on #46312 to land first, and backport the three PRs in one backport PR (i.e. you need to cherry-pick the two other commits onto this branch). I'm going to convert this to draft if that's OK as this can't happen until #46312 lands.

@aduh95 aduh95 marked this pull request as draft January 23, 2023 13:29
@debadree25
Copy link
Member Author

Sure no issue!

@debadree25
Copy link
Member Author

So overall would need to cherry-pick #46205, #46190, #46315 and #46312 into this single PR

@debadree25
Copy link
Member Author

Hi @aduh95 all the streams related changes have landed, should i open a fresh backport PR including all the changes?

@aduh95
Copy link
Contributor

aduh95 commented Feb 27, 2023

You can keep this PR, you just need to push the other commits to your backport-46205-to-v18.x branch.

@debadree25 debadree25 closed this Feb 27, 2023
@debadree25 debadree25 reopened this Feb 27, 2023
@debadree25 debadree25 changed the title [v18.x backport] stream: implement finished() for ReadableStream and WritableStream [v18.x backport] stream: streams and webstreams interop related changes Feb 27, 2023
debadree25
debadree25 commented Feb 27, 2023
View reviewed changes
doc/api/stream.md
@@ -2579,6 +2579,9 @@ further errors except from `_destroy()` may be emitted as `'error'`.
<!-- YAML
added: v10.0.0
changes:
- version: v18.14.0
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 was release in #46396 hence added the version here

@debadree25
Copy link
Member Author

debadree25 commented Feb 27, 2023 •
edited
Loading

Does this look ok @aduh95 ?

@debadree25 debadree25 marked this pull request as ready for review February 28, 2023 10:00
@mshima mshima mentioned this pull request Mar 11, 2023
Closed
1 task
@debadree25 debadree25 requested a review from aduh95 March 12, 2023 11:12
@nodejs-github-bot
Copy link
Collaborator

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

@danielleadams
Copy link
Contributor

@debadree25 are you able to rebase this?

@debadree25
Copy link
Member Author

Sure will rebase this!

@debadree25
lib: add webstreams to Duplex.from()
2c1a619 
Refs: nodejs#39519
PR-URL: nodejs#46190
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@debadree25
doc: include webstreams in finished() and Duplex.from() parameters
631afeb 
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <[email protected]>
@debadree25
stream: remove brandchecks from stream duplexify
0291981 
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@debadree25
Copy link
Member Author

Hey @danielleadams rebased this, is it ok?
So sorry for the delay 😅😅😅

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2023
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced May 24, 2023
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request May 29, 2023
Open
22 tasks
danielleadams pushed a commit that referenced this pull request May 29, 2023
@debadree25 @danielleadams
lib: add webstreams to Duplex.from()
d449b73 
Refs: #39519
PR-URL: #46190
Backport-PR-URL: #46314
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request May 29, 2023
@debadree25 @danielleadams
doc: include webstreams in finished() and Duplex.from() parameters
402c1a3 
PR-URL: #46312
Backport-PR-URL: #46314
Refs: #46190
Refs: #46205
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request May 29, 2023
@debadree25 @danielleadams
stream: remove brandchecks from stream duplexify
e8b5a61 
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Backport-PR-URL: #46314
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danielleadams
Copy link
Contributor

Landed in 0b92035...e8b5a61

This was referenced May 30, 2023
Open
Open
@mshima mshima mentioned this pull request May 31, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielleadams danielleadams danielleadams approved these changes

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@debadree25 @nodejs-github-bot @aduh95 @danielleadams @panva