http: response.setHeaders() by marco-ippolito · Pull Request #46109 · nodejs/node · GitHub

from: #46082
I've extracted from res.writeHead the part where it sets multiple headers into res.setHeaders.
I've removed this part in writeHead:
if (k === undefined && this._header) { throw new ERR_HTTP_HEADERS_SENT('render'); }
because since #45508 it never enters that condition.

Review requested:

@nodejs/http
@nodejs/net

lgtm

One problem that emerged is that I cannot check if (headers instanceof Headers) because importing Headers creates a circular dependency with undici. I need to check it in another way. Maybe if headers has a keys and get function, this would also allow Maps

Co-authored-by: Rich Trott <rtrott@gmail.com>

One problem that emerged is that I cannot check if (headers instanceof Headers) because importing Headers creates a circular dependency with undici. I need to check it in another way. Maybe if headers has a keys and get function, this would also allow Maps

+1 on this. I would also outline in the docs somehow.

Little bit. Rest looks fine.

One problem that emerged is that I cannot check if (headers instanceof Headers) because importing Headers creates a circular dependency with undici. I need to check it in another way. Maybe if headers has a keys and get function, this would also allow Maps

Hmm confused as to why you can't import Headers 😕
Don't know how NodeJS/undici creates a circular dependency. (haven't really dug into the code. But if that's they way it got to be then OK.

Maybe that was the reason why you could not do something similar to:

res.setHeaders = function (headers) {
  if (headers instanceof Headers === false) headers = new Headers(headers)
  if (headers[symbol.toStringTag] !== 'Headers') headers = new Headers(headers) // or this
  
  for (const [key, value] of headers) {
    this.setHeader(key, value)
  }
}

?

@jimmywarting it created a circular dependency because Headers is export by Undici which requires the http module. (the globalThis.Headers is forbidden)
Since Headers is basically an extension of Map I think it was a good idea to allow Map.

PR-URL: nodejs#46109 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

@marco-ippolito this didn't land cleanly on v19.x-staging because you are removing ERR_HTTP_HEADERS_SENT without removing it from the top (the v19.x doesn't include the #45508).

I'll open a backport for you.

Actually, I can't. Your test relies on a feature available only on main requireHostHeader (semver-major). Can you create a manual backport?

PR-URL: nodejs#46109 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

This is breaking test/parallel/test-http-write-head.js in v18.x

/usr/local/opt/python@3.10/bin/python3.10 tools/test.py  --mode=release \                                                                                                                      
                 \                                                                                                                                                                             
                --skip-tests= \                                                                                                                                                                
                default \                                                                                                                                                                      
                addons js-native-api node-api                                                                                                                                                  
=== release test-http-write-head ===                                                                                                                                                           
Path: parallel/test-http-write-head                                                                                                                                                            
node:assert:124                                                                                                                                                                                
  throw new AssertionError(obj);                                                                                                                                                               
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception (Error).
    at Server.<anonymous> (/Users/juan/GitHub/node/test/parallel/test-http-write-head.js:56:10) 
    at Server.<anonymous> (/Users/juan/GitHub/node/test/common/index.js:446:15)
    at Server.emit (node:events:513:28)
    at parserOnIncoming (node:_http_server:1069:12)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: {
    code: 'ERR_HTTP_HEADERS_SENT',
    name: 'Error',
    message: 'Cannot render headers after they are sent to the client'
  },
  operator: 'throws'
}

I have created a manual backport to 19.x #46272, should I create another for 18.x?

I have created a manual backport to 19.x #46272, should I create another for 18.x?

It would great! Thanks!

Backport-PR-URL: #46272 PR-URL: #46109 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

PR-URL: nodejs#46109 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: nodejs#46109 Backport-PR-URL: nodejs#46365 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #46109 Backport-PR-URL: #46365 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

marco-ippolito added 2 commits January 5, 2023 17:45

http: res.setHeaders first implementation

a27f9e5

http: setheaders documentation

e642c57

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jan 5, 2023

http: fix lint

5749b04

mcollina requested changes Jan 5, 2023

View reviewed changes

Comment thread doc/api/http.md Outdated

mscdex reviewed Jan 5, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js Outdated

mscdex reviewed Jan 5, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js Outdated

marco-ippolito added 2 commits January 5, 2023 21:22

http: setheaders accepts headers class

4dceea0

http: writeHead accepts headers

0620d8c

mcollina requested changes Jan 5, 2023

View reviewed changes

Comment thread doc/api/http.md Outdated

ShogunPanda reviewed Jan 6, 2023

View reviewed changes

Comment thread lib/_http_server.js Outdated

Comment thread lib/_http_outgoing.js Outdated

marco-ippolito added 2 commits January 6, 2023 12:44

http: removed support for array and object

680904d

http: fix comments lint

67a0838

marco-ippolito requested a review from ShogunPanda January 6, 2023 11:54

ShogunPanda reviewed Jan 6, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js

ShogunPanda reviewed Jan 6, 2023

View reviewed changes

Comment thread lib/_http_server.js

mcollina approved these changes Jan 6, 2023

View reviewed changes

Trott reviewed Jan 6, 2023

View reviewed changes

Comment thread test/parallel/test-http-response-setheaders.js Outdated

http: fix typo

0075cbd

Co-authored-by: Rich Trott <rtrott@gmail.com>

mscdex reviewed Jan 7, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js Outdated

http: added map support

eb7481f

ShogunPanda approved these changes Jan 7, 2023

View reviewed changes

Comment thread doc/api/http.md Outdated

mscdex reviewed Jan 7, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js Outdated

marco-ippolito added 2 commits January 7, 2023 14:34

http: fix array test

822eb15

http: fix lint

e33a41c

anonrig reviewed Jan 8, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js Outdated

This was referenced Jan 11, 2023

CI Reliability 2023-01-11 nodejs/reliability#480

Open

CI Reliability 2023-01-12 nodejs/reliability#481

Open

CI Reliability 2023-01-13 nodejs/reliability#482

Open

marco-ippolito mentioned this pull request Jan 13, 2023

Allow passing new Headers() to res.writeHead(...) #46082

Closed

This was referenced Jan 14, 2023

CI Reliability 2023-01-14 nodejs/reliability#483

Open

CI Reliability 2023-01-15 nodejs/reliability#484

Open

CI Reliability 2023-01-16 nodejs/reliability#485

Open

CI Reliability 2023-01-17 nodejs/reliability#486

Open

RafaelGSS added the backport-requested-v19.x label Jan 17, 2023

marco-ippolito mentioned this pull request Jan 19, 2023

[v19.x backport] http: res set headers #46272

Closed

juanarbol added the backport-requested-v18.x label Jan 25, 2023

marco-ippolito mentioned this pull request Jan 26, 2023

[18.x backport] http: res setheaders #46365

Closed

ruyadorno mentioned this pull request Feb 1, 2023

v19.6.0 proposal #46455

Merged

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

targos added backported-to-v19.x and removed backport-requested-v19.x labels Mar 13, 2023

http: response.setHeaders() by marco-ippolito · Pull Request #46109 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: response.setHeaders() - #46109

http: response.setHeaders()#46109nodejs-github-bot merged 12 commits intonodejs:mainnodejs/node:mainfrom marco-ippolito:feat/res-setheadersmarco-ippolito/node:feat/res-setheadersCopy head branch name to clipboard

Conversation

marco-ippolito commented Jan 5, 2023

Uh oh!

nodejs-github-bot commented Jan 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

marco-ippolito commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ShogunPanda commented Jan 7, 2023

Uh oh!

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimmywarting commented Jan 13, 2023

Uh oh!

marco-ippolito commented Jan 13, 2023

Uh oh!

RafaelGSS commented Jan 17, 2023

Uh oh!

RafaelGSS commented Jan 17, 2023

Uh oh!

juanarbol commented Jan 25, 2023

Uh oh!

marco-ippolito commented Jan 25, 2023

Uh oh!

juanarbol commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

http: response.setHeaders()#46109
nodejs-github-bot merged 12 commits into
nodejs:mainfrom
marco-ippolito:feat/res-setheaders

marco-ippolito commented Jan 6, 2023 •
edited
Loading