Sitelet https://web.archive.org/web/20220323021425/https://github.com/nodejs/node/pull/35924
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

http: enable setHeader for call chaining #35924

Merged
merged 0 commits into from Nov 30, 2020
Merged

http: enable setHeader for call chaining #35924

merged 0 commits into from Nov 30, 2020

Conversation

PoojaDurgad
Copy link
Contributor

@PoojaDurgad PoojaDurgad commented Nov 2, 2020 •
edited

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 2, 2020

Review requested:

@nodejs-github-bot nodejs-github-bot added the http label Nov 2, 2020
@mscdex
Copy link
Contributor

@mscdex mscdex commented Nov 2, 2020

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

@mscdex mscdex changed the title lib,http: enable setHeader for call chaining http: enable setHeader for call chaining Nov 2, 2020
lpinca
lpinca reviewed Nov 2, 2020
View changes
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

@lpinca lpinca commented Nov 2, 2020

Can you please add a test?

lpinca
lpinca approved these changes Nov 3, 2020
View changes
Copy link
Member

@lpinca lpinca left a comment •
edited

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.

test/parallel/test-http-set-header-chain.js Outdated Show resolved Hide resolved
test/parallel/test-http-set-header-chain.js Outdated Show resolved Hide resolved
rickyes
rickyes approved these changes Nov 5, 2020
View changes
@rickyes rickyes added the request-ci label Nov 5, 2020
@github-actions github-actions bot removed the request-ci label Nov 5, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 5, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 5, 2020

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

@Trott Trott added the semver-minor label Nov 5, 2020
Trott
Trott approved these changes Nov 5, 2020
View changes
@Trott
Copy link
Member

@Trott Trott commented Nov 5, 2020 •
edited

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

@Trott Trott added the request-ci label Nov 5, 2020
@github-actions github-actions bot removed the request-ci label Nov 5, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 5, 2020

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

@Trott
Copy link
Member

@Trott Trott commented Nov 5, 2020

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
Copy link
Member

@Trott Trott commented Nov 5, 2020

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

@PoojaDurgad
Copy link
Contributor Author

@PoojaDurgad PoojaDurgad commented Nov 5, 2020

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!!

@Trott
Copy link
Member

@Trott Trott commented Nov 6, 2020

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 (***)

@PoojaDurgad
Copy link
Contributor Author

@PoojaDurgad PoojaDurgad commented Nov 27, 2020

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

@Trott Trott added notable-change and removed notable-change labels Nov 28, 2020
@Trott Trott added the request-ci label Nov 28, 2020
@github-actions github-actions bot removed the request-ci label Nov 28, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 28, 2020

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

@Trott
Copy link
Member

@Trott Trott commented Nov 30, 2020

Landed in dedd061

@Trott Trott closed this Nov 30, 2020
@Trott Trott merged commit dedd061 into nodejs:master Nov 30, 2020
9 checks passed
danielleadams added a commit that referenced this issue Dec 7, 2020
@danielleadams
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 <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
Merged
danielleadams added a commit that referenced this issue Dec 7, 2020
@danielleadams
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)
cjihrig added a commit to cjihrig/node that referenced this issue Dec 8, 2020
@cjihrig
http: enable call chaining with setHeader()
df3d09c 
Make `response.setHeader` return the response object itself
so that multiple header setting can be chained.

Fixes: nodejs#33148

PR-URL: nodejs#35924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams added a commit that referenced this issue Dec 9, 2020
@danielleadams
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 issue Dec 9, 2020
@danielleadams
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 added a commit that referenced this issue May 1, 2021
@targos
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 <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott

@lpinca lpinca

@rickyes rickyes

Assignees
No one assigned
Labels
http semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

respose.setHeader() should return response to allow chaining
6 participants
@PoojaDurgad @nodejs-github-bot @mscdex @lpinca @Trott @rickyes