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

Makes http(2) response.writehead return this #25974

Closed
wants to merge 7 commits into from
Closed

Makes http(2) response.writehead return this #25974

wants to merge 7 commits into from

Conversation

qubyte
Copy link
Contributor

@qubyte qubyte commented Feb 7, 2019

Addresses #25935.

I chose in the end to go with the lightest touch and only update writeHead (this is the most useful to change in my experience). I'm happy to extend this to res.setHeader too, but it may suggest updating other similar methods (flushHeaders, removeHeader, etc.)

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 nodejs-github-bot added dont-land-on-v6.x http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. labels Feb 7, 2019
qubyte
qubyte commented Feb 7, 2019
View reviewed changes
lib/internal/http2/compat.js
@@ -568,10 +568,8 @@ class Http2ServerResponse extends Stream {
if (this[kStream].headersSent)
throw new ERR_HTTP2_HEADERS_SENT();

// If the stream is destroyed, we return false,
// like require('http').
Copy link
Contributor Author

@qubyte qubyte Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't find the code which this refers to. The HTTP version of writeHead appears to have no explicit returns.

addaleax
addaleax approved these changes Feb 7, 2019
View reviewed changes
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
addaleax and others added 2 commits February 7, 2019 00:55
@addaleax @qubyte
http: update to documentation
1ce1dcc 
Addresses PR feedback.

Co-Authored-By: qubyte <mark.s.everitt@gmail.com>
@addaleax @qubyte
http2: update to documentation
3a4d057 
Addresses PR feedback.

Co-Authored-By: qubyte <mark.s.everitt@gmail.com>
vsemozhetbyt
vsemozhetbyt reviewed Feb 7, 2019
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Thank you. Some doc nits.

doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
vsemozhetbyt and others added 2 commits February 7, 2019 01:39
@vsemozhetbyt @qubyte
http2: update to documentation
2f476b6 
Addresses PR feedback.

Co-Authored-By: qubyte <mark.s.everitt@gmail.com>
@vsemozhetbyt @qubyte
http: update to documentation
c5d4090 
Addresses PR feedback.

Co-Authored-By: qubyte <mark.s.everitt@gmail.com>
@vsemozhetbyt @qubyte
http2: update to documentation
df45bdc 
Addresses PR feedback.

Co-Authored-By: qubyte <mark.s.everitt@gmail.com>
@lpinca
Copy link
Member

lpinca commented Feb 9, 2019

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

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 9, 2019
@addaleax
Copy link
Member

addaleax commented Feb 9, 2019

Landed in f93df51, 1aa11e1 – thanks for the PR! 🎉

@addaleax addaleax closed this Feb 9, 2019
addaleax pushed a commit that referenced this pull request Feb 9, 2019
@qubyte @addaleax
http: makes response.writeHead return the response
f93df51 
Fixes: #25935

PR-URL: #25974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 9, 2019
@qubyte @addaleax
http2: makes response.writeHead return the response
1aa11e1 
Fixes: #25935

PR-URL: #25974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@qubyte
Copy link
Contributor Author

qubyte commented Feb 9, 2019

Thanks for accepting! 😁

targos pushed a commit that referenced this pull request Feb 10, 2019
@qubyte @targos
http: makes response.writeHead return the response
a5247cc 
Fixes: #25935

PR-URL: #25974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Feb 10, 2019
@qubyte @targos
http2: makes response.writeHead return the response
237b5e6 
Fixes: #25935

PR-URL: #25974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@qubyte qubyte deleted the http-response-writehead-return-this branch February 27, 2019 22:54
BethGriggs pushed a commit that referenced this pull request Oct 7, 2019
@qubyte @BethGriggs
http: makes response.writeHead return the response
ccf2823 
Fixes: #25935

PR-URL: #25974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 7, 2019
@qubyte @BethGriggs
http2: makes response.writeHead return the response
9258496 
Fixes: #25935

PR-URL: #25974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 7, 2019
Merged
BethGriggs added a commit that referenced this pull request Oct 22, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
5dae3ef 
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 22, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
afbbcb0 
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 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.
6 participants
@qubyte @lpinca @addaleax @vsemozhetbyt @BethGriggs @nodejs-github-bot