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

Jump to bottom

lib: allow http.OutgoingMessage to be used in Writable.toWeb() #45642

Merged
merged 5 commits into from Dec 10, 2022
Merged

lib: allow http.OutgoingMessage to be used in Writable.toWeb() #45642

merged 5 commits into from Dec 10, 2022

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Nov 27, 2022 •
edited

Attempted to fix the issue by watering down the condition being checked in lib/internal/webstreams/adapters.js similar to
isWritableNodeStream utility being used in internal/streams/utils

Fixes: #44188

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Nov 27, 2022
@debadree25 debadree25 changed the title Fix/to-web-allow-server-reponse allow http.OutgoingMessage to be used in Writable.toWeb() Nov 27, 2022
@debadree25 debadree25 changed the title allow http.OutgoingMessage to be used in Writable.toWeb() src: allow http.OutgoingMessage to be used in Writable.toWeb() Nov 27, 2022
@debadree25
Copy link
Member Author

@cjihrig have updated the tests do review!

@cjihrig
Copy link
Contributor

cjihrig commented Nov 27, 2022

Thanks. I've started the GitHub Actions CI for you.

@debadree25
Copy link
Member Author

It appears some of the CI tests are failing working on fixing them

@debadree25
Copy link
Member Author

Ok it seems that the changes i made are resulting in issues with duplexes somehow, trying to get to the bottom of this, if anybody has any insight on this would love to hear

Thank you!

@debadree25 debadree25 force-pushed the fix/to-web-allow-server-reponse branch from f73e049 to 190611b Compare November 28, 2022 15:44
@debadree25 debadree25 changed the title src: allow http.OutgoingMessage to be used in Writable.toWeb() lib: allow http.OutgoingMessage to be used in Writable.toWeb() Nov 28, 2022
@debadree25
Copy link
Member Author

Have linted the commit message and fixed the reasons causing the tests with duplexes to fail, we can have another CI run

Thank You

@debadree25
Copy link
Member Author

I guess we could have CI on this PR?

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 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. labels Dec 10, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2022
@nodejs-github-bot nodejs-github-bot merged commit 2ec4189 into nodejs:main Dec 10, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 2ec4189

ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Dec 12, 2022
@debadree25 @ErickWendel
lib: allow Writeable.toWeb() to work on http.Outgoing message
4f25c2e 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: nodejs#44188
PR-URL: nodejs#45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this pull request Dec 12, 2022
@debadree25 @targos
lib: allow Writeable.toWeb() to work on http.Outgoing message
3c7ac01 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos mentioned this pull request Dec 12, 2022
Merged
targos pushed a commit that referenced this pull request Dec 13, 2022
@debadree25 @targos
lib: allow Writeable.toWeb() to work on http.Outgoing message
302c524 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@debadree25 @danielleadams
lib: allow Writeable.toWeb() to work on http.Outgoing message
1a424b9 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@debadree25 @danielleadams
lib: allow Writeable.toWeb() to work on http.Outgoing message
c966a3d 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@debadree25 @danielleadams
lib: allow Writeable.toWeb() to work on http.Outgoing message
bbaca84 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@debadree25 @danielleadams
lib: allow Writeable.toWeb() to work on http.Outgoing message
62254cb 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
@debadree25 @danielleadams
lib: allow Writeable.toWeb() to work on http.Outgoing message
b453d4c 
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

http.ServerResponse is not an instance of stream.Writable?
6 participants
@debadree25 @cjihrig @nodejs-github-bot @jasnell @anonrig @aduh95