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

[v20.x backport] streams: fixes for webstreams #52773

Closed
wants to merge 1 commit into from
Closed

[v20.x backport] streams: fixes for webstreams #52773

wants to merge 1 commit into from

Conversation

MattiasBuelens
Copy link
Contributor

Manual backport of #51168 to v20.x.
Depends on #52772, so keeping this in draft until that one lands.

This one was a bit tricky, since there are changes to transferables on v22.x that are not on v20.x (such as #50107) which also affect web streams. However, I think I made it work. 🤞

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. web streams labels May 1, 2024
@MattiasBuelens MattiasBuelens force-pushed the backport-51168-to-v20.x branch 2 times, most recently from 43c8f92 to a7758ee Compare May 1, 2024 11:18
@MattiasBuelens MattiasBuelens mentioned this pull request May 1, 2024
Merged
@marco-ippolito
Copy link
Member

can you fix the conflicts? I'll try to backport it in the next v20 release

@MattiasBuelens MattiasBuelens marked this pull request as ready for review May 16, 2024 18:27
@MattiasBuelens
Copy link
Contributor Author

@marco-ippolito Updated and ready for review. 😉

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@marco-ippolito
Copy link
Member

marco-ippolito commented Jun 12, 2024 •
edited
Loading

Hi @MattiasBuelens I was backporting your commits, I encountered some issue, can you please cherry-pick the landed commits from the original PR (example #51168 (comment)), instead of the commits from your fork, so that they contain metadata like PR-URL and reviewers, and please squash the fixup commits

@marco-ippolito marco-ippolito force-pushed the v20.x-staging branch 3 times, most recently from a924e20 to 473fa73 Compare June 19, 2024 09:12
@MattiasBuelens
stream: fixes for webstreams
573f5df 
PR-URL: nodejs#51168
Backport-PR-URL: nodejs#52773
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MattiasBuelens
Copy link
Contributor Author

@marco-ippolito I've squashed the commits on this PR and rebased them onto v20.x-staging.

The original PR didn't have the most prominent change in the first commit, so we landed it in 20c6313 with the unimaginative title "stream: fix code style". 😅 I changed it to "stream: fixes for webstreams" for this PR, if that's okay.

@marco-ippolito
Copy link
Member

@marco-ippolito I've squashed the commits on this PR and rebased them onto v20.x-staging.

The original PR didn't have the most prominent change in the first commit, so we landed it in 20c6313 with the unimaginative title "stream: fix code style". 😅 I changed it to "stream: fixes for webstreams" for this PR, if that's okay.

You can just cherry-pick the landed commits from the original PR (at the end of the pr in the comment from the bot landed in ...) and squash only the fixup commits

mcollina
mcollina approved these changes Jul 1, 2024
View reviewed changes
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

marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@MattiasBuelens @marco-ippolito
stream: fixes for webstreams
48138af 
PR-URL: #51168
Backport-PR-URL: #52773
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@marco-ippolito
Copy link
Member

Landed in 48138af

@MattiasBuelens MattiasBuelens deleted the backport-51168-to-v20.x branch July 25, 2024 16:25
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

@marco-ippolito marco-ippolito marco-ippolito approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@MattiasBuelens @nodejs-github-bot @marco-ippolito @mcollina