GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — http: enable setHeader for call chaining by PoojaDurgad · Pull Request #35924 · nodejs/node

Conversation

PoojaDurgad commented Nov 2, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Make response.setHeader return the response object itself
so that multiple header setting can be chained.

Fixes: #33148

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji

All reactions

👀 1 reaction

nodejs-github-bot commented Nov 2, 2020

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/http
@nodejs/net

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added the http Issues and PRs related to the http subsystem. label

Nov 2, 2020

mscdex commented Nov 2, 2020

Copy link

Copy Markdown

Contributor

The targeted subsystem can just be http in the commit message instead of lib,http since http is the only thing being changed here.

PoojaDurgad reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PoojaDurgad force-pushed the setheader branch from c8fa3aa to 345fc6c Compare

November 2, 2020 15:58

mscdex changed the title ~~lib,http: enable setHeader for call chaining~~ http: enable setHeader for call chaining

Nov 2, 2020

lpinca reviewed

Nov 2, 2020

View reviewed changes

Comment thread

doc/api/http.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

doc/api/http.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Nov 2, 2020

Copy link

Copy Markdown

Member

Can you please add a test?

PoojaDurgad reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca approved these changes

Nov 3, 2020

View reviewed changes

lpinca left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the test file as is but it could be simplified to something like

'use strict';
require('../common');
const assert = require('assert');
const { ServerResponse } = require('http');

const response = new ServerResponse({
  method: 'GET',
  httpVersionMajor: 1,
  httpVersionMinor: 1
});

assert.strictEqual(response.setHeader('foo', 'bar'), response);

or just a single assertion like the one above in an already existing test where response.setHeader() is used.

There is no need to start a server and chaining is a logical consequence of returning the same object.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-http-set-header-chain.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

test/parallel/test-http-set-header-chain.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

rickyes approved these changes

Nov 5, 2020

View reviewed changes

rickyes added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 5, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 5, 2020

nodejs-github-bot commented Nov 5, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Nov 5, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PoojaDurgad force-pushed the setheader branch from 5984558 to 3880ce6 Compare

November 5, 2020 12:49

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label

Nov 5, 2020

Trott approved these changes

Nov 5, 2020

View reviewed changes

Trott commented Nov 5, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Nit on the commit message. Maybe this is a little more clear?: http: enable call chaining with setHeader() Or if you don't like that, maybe this?: http: enable setHeader() call chaining

PoojaDurgad reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 5, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 5, 2020

nodejs-github-bot commented Nov 5, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Nov 5, 2020

Copy link

Copy Markdown

Member

Will this automatically add this for https as well? If so, should we add a test? If not, should we also add it to https?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Nov 5, 2020

Copy link

Copy Markdown

Member

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/680/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PoojaDurgad force-pushed the setheader branch from 3880ce6 to be03927 Compare

November 5, 2020 15:19

PoojaDurgad commented Nov 5, 2020

Copy link

Copy Markdown

Contributor Author

Will this automatically add this for https as well? If so, should we add a test? If not, should we also add it to https?

@Trott - This behavior will be added to the https and need a test I guess . I think this feature can also be added to the http2 module too!!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Nov 6, 2020

Copy link

Copy Markdown

Member

Benchmark results show a small performance hit in some cases. Probably acceptable? @nodejs/http

07:04:12                                                                    confidence improvement accuracy (*)   (**)  (***)
07:04:12  http/set-header.js duration=5 res='normal' benchmarker='wrk'                     -2.36 %       ±4.51% ±6.01% ±7.84%
07:04:12  http/set-header.js duration=5 res='setHeader' benchmarker='wrk'            *     -3.48 %       ±3.17% ±4.22% ±5.49%
07:04:12  http/set-header.js duration=5 res='setHeaderWH' benchmarker='wrk'                -2.83 %       ±3.86% ±5.14% ±6.71%
07:04:12  http/set_header.js n=1000000 value='Connection'                                  -2.96 %       ±4.18% ±5.62% ±7.44%
07:04:12  http/set_header.js n=1000000 value='Content-Length'                              -0.92 %       ±2.31% ±3.08% ±4.03%
07:04:12  http/set_header.js n=1000000 value='Content-Type'                                 0.05 %       ±1.31% ±1.74% ±2.27%
07:04:12  http/set_header.js n=1000000 value='Set-Cookie'                                   0.75 %       ±1.57% ±2.09% ±2.72%
07:04:12  http/set_header.js n=1000000 value='Transfer-Encoding'                           -0.75 %       ±3.16% ±4.22% ±5.54%
07:04:12  http/set_header.js n=1000000 value='Vary'                                  *     -2.19 %       ±1.95% ±2.61% ±3.41%
07:04:12  http/set_header.js n=1000000 value='X-Powered-By'                                 0.95 %       ±1.86% ±2.48% ±3.23%
07:04:12 
07:04:12 Be aware that when doing many comparisons the risk of a false-positive
07:04:12 result increases. In this case there are 10 comparisons, you can thus
07:04:12 expect the following amount of false-positive results:
07:04:12   0.50 false positives, when considering a   5% risk acceptance (*, **, ***),
07:04:12   0.10 false positives, when considering a   1% risk acceptance (**, ***),
07:04:12   0.01 false positives, when considering a 0.1% risk acceptance (***)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PoojaDurgad commented Nov 27, 2020

Copy link

Copy Markdown

Contributor Author

is there anything pending on this PR? if yes let me know .

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott added notable-change PRs with changes that should be highlighted in changelogs. and removed notable-change PRs with changes that should be highlighted in changelogs. labels

Nov 28, 2020

Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 28, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 28, 2020

nodejs-github-bot commented Nov 28, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Nov 30, 2020

Copy link

Copy Markdown

Member

Landed in dedd061

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott closed this

Nov 30, 2020

Trott force-pushed the setheader branch from be03927 to dedd061 Compare

November 30, 2020 13:15

Trott merged commit dedd061 into nodejs:master

Nov 30, 2020

danielleadams pushed a commit that referenced this pull request

Dec 7, 2020


          http: enable call chaining with setHeader()

58abdca

Make `response.setHeader` return the response object itself
so that multiple header setting can be chained.

Fixes: #33148

PR-URL: #35924
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

danielleadams mentioned this pull request

Dec 7, 2020

v15.4.0 proposal #36435

Merged

danielleadams added a commit that referenced this pull request

Dec 7, 2020


          2020-12-08, Version 15.4.0 (Current)

fde09a4

PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)

danielleadams added a commit that referenced this pull request

Dec 9, 2020


          2020-12-09, Version 15.4.0 (Current)

5e5927e

PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)

danielleadams added a commit that referenced this pull request

Dec 9, 2020


          2020-12-09, Version 15.4.0 (Current)

348b7a9

PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)

targos pushed a commit that referenced this pull request

May 1, 2021


          http: enable call chaining with setHeader()

ad39b37

Make `response.setHeader` return the response object itself
so that multiple header setting can be chained.

Fixes: #33148

PR-URL: #35924
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

danielleadams mentioned this pull request

May 3, 2021

v14.17.0 proposal #38507

Merged

coderabbitai Bot mentioned this pull request

Aug 17, 2026

docs: point response.set setHeader link at current Node.js docs koajs/koa#1995

Open

Reviewers

Trott

Trott approved these changes

lpinca

lpinca approved these changes

+1 more reviewer

rickyes

rickyes approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http

Issues and PRs related to the http subsystem.

semver-minor

PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

respose.setHeader() should return response to allow chaining

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

PoojaDurgad commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Nov 2, 2020

Uh oh!

mscdex commented Nov 2, 2020

Uh oh!

Uh oh!

Uh oh!

lpinca commented Nov 2, 2020

Uh oh!

lpinca left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 5, 2020

Uh oh!

nodejs-github-bot commented Nov 5, 2020

Uh oh!

Trott commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 5, 2020

Uh oh!

Trott commented Nov 5, 2020

Uh oh!

Trott commented Nov 5, 2020

Uh oh!

PoojaDurgad commented Nov 5, 2020

Uh oh!

Trott commented Nov 6, 2020

Uh oh!

PoojaDurgad commented Nov 27, 2020

Uh oh!

nodejs-github-bot commented Nov 28, 2020

Uh oh!

Trott commented Nov 30, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PoojaDurgad commented Nov 2, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

lpinca left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Nov 5, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.