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

http: writeHead if statusmessage is undefined dont override headers #46173

Merged
merged 2 commits into from
Jan 17, 2023
Merged

http: writeHead if statusmessage is undefined dont override headers #46173

merged 2 commits into from
Jan 17, 2023

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Jan 11, 2023 •
edited
Loading

fix: #32395
this PR avoids that when calling res.writeHead(200, undefined, { 'x-foo': 'bar' }); the headers are ignored.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jan 11, 2023
anonrig
anonrig approved these changes Jan 11, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig 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 time. although, next time, I recommend separating refactoring and the actual task into 2 different commits, so it would be easier to differentiate (on a particularly large pull request), what the proposed solution is

mscdex
mscdex reviewed Jan 11, 2023
View reviewed changes
lib/_http_server.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

anonrig
anonrig reviewed Jan 11, 2023
View reviewed changes
lib/_http_server.js Outdated Show resolved Hide resolved
This was referenced Jan 12, 2023
Open
Open
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2023
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Jan 13, 2023
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

@github-actions github-actions bot mentioned this pull request Jan 14, 2023
Open
19 tasks
@ShogunPanda ShogunPanda 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 Jan 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6e375b3 into nodejs:main Jan 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6e375b3

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
@marco-ippolito @RafaelGSS
http: writeHead if statusmessage is undefined dont override headers
66a2277 
PR-URL: nodejs#46173
Fixes: nodejs#32395
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@marco-ippolito @RafaelGSS
http: writeHead if statusmessage is undefined dont override headers
ae5141c 
PR-URL: #46173
Fixes: #32395
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@marco-ippolito @juanarbol
http: writeHead if statusmessage is undefined dont override headers
aa1cb80 
PR-URL: #46173
Fixes: #32395
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@marco-ippolito @juanarbol
http: writeHead if statusmessage is undefined dont override headers
3e70b7d 
PR-URL: #46173
Fixes: #32395
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@jasnell jasnell jasnell approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@mcollina mcollina mcollina approved these changes

@mscdex mscdex Awaiting requested review from mscdex

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

response.writeHead() does not default statusMessage if it's undefined
7 participants
@marco-ippolito @nodejs-github-bot @mcollina @mscdex @ShogunPanda @jasnell @anonrig