doc: improve http.setHeader and getHeader typeinfo #19902

Flarna · 2018-04-09T21:46:22Z

http.setHeader() coerces input values to strings.
http.getHeader() returns the type as passed to setHeader().

Fixes: #13825

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

http.setHeader() coerces input values. http.getHeader() returns the type as passed to setHeader() fixes #13825

Trott · 2018-04-09T21:56:28Z

@nodejs/documentation @nodejs/http

Trott · 2018-04-09T21:58:30Z

doc/api/http.md

@@ -587,9 +587,11 @@ added: v1.6.0
 -->

 * `name` {string}
-* Returns: {string}
+* Returns: {string, string[]} - type as set via `setHeader()`


The type as set via part seems out of place. If the information is critical, it can be included in the text below.

Use a | to separate the two possible return types, not a comma. (This is to be consistent with our documentation elsewhere.)

@vsemozhetbyt Yes, thanks, updated!

Trott

Thanks for the PR! A few suggested changes...

Trott · 2018-04-09T22:00:58Z

doc/api/http.md

@@ -1210,7 +1212,8 @@ added: v0.4.0

 Sets a single header value for implicit headers. If this header already exists
 in the to-be-sent headers, its value will be replaced. Use an array of strings
-here to send multiple headers with the same name.
+here to send multiple headers with the same name. Values are coerced to
+strings if needed during transmission.


Perhaps just this for the last sentence? Non-string values will be coerced to strings.

Trott · 2018-04-09T22:01:14Z

doc/api/http.md


 Reads out a header on the request. Note that the name is case insensitive.
+The return value is that one set via `setHeader()` as coercing to string
+happens during transmission.


This sentence is a bit difficult to understand. How about this instead:

The type of the return value depends on the arguments provided to `setHeader()`.

Also, setHeaders() should be a link to that function in the docs.

@vsemozhetbyt Whoops, yes, thanks for the correction!

Flarna · 2018-04-10T08:08:53Z

Updated.
Just for the notes. I considered also to change the http implementation to match the docu by doing the conversion to strings in setHeader. But I found that current behavior is verified in some test and I fear it would break at least one app/module out there.

vsemozhetbyt · 2018-04-10T08:43:42Z

doc/api/http.md


 Example:
 ```js
-const contentType = request.getHeader('Content-Type');
+request.setHeader('content-type', 'text/html');
+request.setHeader('Content-Length': Buffer.byteLength(body));


vsemozhetbyt · 2018-04-10T08:43:49Z

doc/api/http.md

-const contentType = request.getHeader('Content-Type');
+request.setHeader('content-type', 'text/html');
+request.setHeader('Content-Length': Buffer.byteLength(body));
+request.setHeader('Set-Cookie', ['type=ninja', 'language=javascript'])


Missing semicolon.

vsemozhetbyt · 2018-04-10T08:46:24Z

doc/api/http.md

+request.setHeader('content-type', 'text/html');
+request.setHeader('Content-Length': Buffer.byteLength(body));
+request.setHeader('Set-Cookie', ['type=ninja', 'language=javascript'])
+const contentType = request.getHeader('Content-Type');      // contentType is 'text/html'


We lint code examples in docs, so these 3 lines will violate max-len rule (they exceed 80 characters length). The comments may be moved to their own lines.

vsemozhetbyt · 2018-04-10T08:47:58Z

doc/api/http.md

+request.setHeader('Content-Length': Buffer.byteLength(body));
+request.setHeader('Set-Cookie', ['type=ninja', 'language=javascript'])
+const contentType = request.getHeader('Content-Type');      // contentType is 'text/html'
+const contentLength = request.getHeader('Content-Length');  // contentLength is of type number


If this is true, should the parameter types be updated? Can we define the whole possible set there?

Well, it's javascript so mostly everything is coerced to a string (even undefined). So actually setHeader() accepts any type and as a result getHeader() returns any type as it is a pass through.

So maybe we can use {any} there? @Trott, what do you think?

So maybe we can use {any} there? @Trott, what do you think?

Works for me. Easy enough to update it to something else if we discover this confuses a lot of people. I don't think it will, though.

vsemozhetbyt · 2018-04-10T09:14:08Z

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

MoonBall · 2018-04-10T10:01:58Z

I considered also to change the http implementation to match the docu by doing the conversion to strings in setHeader.

@Flarna The current behavior is expected. Non-string values will be coerced to strings in order to transmit them.

MoonBall · 2018-04-10T10:04:24Z

doc/api/http.md

@@ -587,13 +587,24 @@ added: v1.6.0
 -->

 * `name` {string}
-* Returns: {string}
+* Returns: {string|string[]}


Should this be * Returns: {any}? Because

The type of the return value depends on the arguments provided to [response.setHeader()][].

Should we use any also for setHeader then?
It is allowed and works as long as it translates to a string on the fly. It would be strange to say getHeader returns what was provided to setHeader but doc says setHeader accepts only string|string[] but getHeader returns any.
Something like stringifyable would be best match but this is no type.

MoonBall · 2018-04-10T10:06:45Z

doc/api/http.md

@@ -1210,7 +1221,8 @@ added: v0.4.0

 Sets a single header value for implicit headers. If this header already exists
 in the to-be-sent headers, its value will be replaced. Use an array of strings
-here to send multiple headers with the same name.
+here to send multiple headers with the same name. Non-string values will be
+coerced to strings.


Non-string values will be coerced to strings in order to transmit them.

They are coerced to strings already during set in validateHeader() => checkInvalidHeaderChar() but the result is not stored. if it fails or holds invalid chars caller gets an Error. During sending they are converted once more.

Ooof, I see the subtlety here now. Maybe be super explicit?

Non-string values will be stored without modification. Therefore, `getHeader()` may return non-string values. However, the non-string values will be converted to strings for network transmission.

Something like that maybe (but wrapped at 80 chars instead whatever I'm wrapping at above)?

my original concern has been addressed

Flarna · 2018-04-10T22:21:40Z

Updated once more with hints from @Trott. I noticed also that I mixed up request/response APIs. Now both of the should be updated.

MoonBall · 2018-04-11T07:16:26Z

doc/api/http.md

@@ -616,11 +627,14 @@ added: v1.6.0
 -->

 * `name` {string}
-* `value` {string}
+* `value` {string|string[]}


The type of value should be any.

The response.setHeader() also needs to be modified.

LGTM if the above two small nits are fixed. @Flarna

BridgeAR · 2018-04-12T14:36:55Z

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

trivikr · 2018-04-12T16:39:56Z

CI-Lite: https://ci.nodejs.org/job/node-test-pull-request-lite/503/

http.setHeader() coerces input values. http.getHeader() returns the type as passed to setHeader(). PR-URL: nodejs#19902 Fixes: nodejs#13825 Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-04-13T18:28:49Z

Landed in 50e9f8d 🎉

http.setHeader() coerces input values. http.getHeader() returns the type as passed to setHeader(). PR-URL: #19902 Fixes: #13825 Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

http.setHeader() coerces input values. http.getHeader() returns the type as passed to setHeader(). PR-URL: nodejs#19902 Fixes: nodejs#13825 Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Apr 9, 2018

Flarna changed the title ~~doc: improve http.setHeader and getHeader typinfo~~ doc: improve http.setHeader and getHeader typeinfo Apr 9, 2018

doc: improve http.setHeader and getHeader typeinfo

3699198

http.setHeader() coerces input values. http.getHeader() returns the type as passed to setHeader() fixes #13825

Trott reviewed Apr 9, 2018

View reviewed changes

Trott previously requested changes Apr 9, 2018

View reviewed changes

BridgeAR requested a review from vsemozhetbyt April 9, 2018 23:30

fix review items

32685cf

vsemozhetbyt reviewed Apr 10, 2018

View reviewed changes

fix review items

1d5c3e2

MoonBall reviewed Apr 10, 2018

View reviewed changes

MoonBall suggested changes Apr 10, 2018

View reviewed changes

gireeshpunathil mentioned this pull request Apr 10, 2018

getHeader() returns undefined after using writeHead() in a certain circumstance #10354

Closed

fix review findings, correct request and response APIs

5dc2a5d

MoonBall reviewed Apr 11, 2018

View reviewed changes

MoonBall approved these changes Apr 11, 2018

View reviewed changes

setHeader accepts type any

3ab5dc7

BridgeAR approved these changes Apr 12, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018

trivikr approved these changes Apr 12, 2018

View reviewed changes

jasnell approved these changes Apr 12, 2018

View reviewed changes

BridgeAR closed this Apr 13, 2018

Flarna deleted the http_doc_getheader branch April 13, 2018 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: improve http.setHeader and getHeader typeinfo #19902

doc: improve http.setHeader and getHeader typeinfo #19902

Flarna commented Apr 9, 2018 •

edited by BridgeAR

Loading

Trott commented Apr 9, 2018

Trott Apr 9, 2018 •

edited

Loading

Trott Apr 10, 2018

Trott left a comment

Trott Apr 9, 2018

Trott Apr 9, 2018 •

edited

Loading

Trott Apr 10, 2018

Flarna commented Apr 10, 2018

vsemozhetbyt Apr 10, 2018

vsemozhetbyt Apr 10, 2018

vsemozhetbyt Apr 10, 2018

vsemozhetbyt Apr 10, 2018

Flarna Apr 10, 2018

vsemozhetbyt Apr 10, 2018

Trott Apr 10, 2018

vsemozhetbyt commented Apr 10, 2018

MoonBall commented Apr 10, 2018 •

edited

Loading

MoonBall Apr 10, 2018 •

edited

Loading

Flarna Apr 10, 2018 •

edited

Loading

MoonBall Apr 10, 2018 •

edited

Loading

Flarna Apr 10, 2018 •

edited

Loading

Trott Apr 10, 2018 •

edited

Loading

Flarna commented Apr 10, 2018

MoonBall Apr 11, 2018

MoonBall Apr 11, 2018

MoonBall Apr 11, 2018

BridgeAR commented Apr 12, 2018

trivikr commented Apr 12, 2018

BridgeAR commented Apr 13, 2018

doc: improve http.setHeader and getHeader typeinfo #19902

doc: improve http.setHeader and getHeader typeinfo #19902

Conversation

Flarna commented Apr 9, 2018 • edited by BridgeAR Loading

Checklist

Trott commented Apr 9, 2018

Trott Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flarna commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 10, 2018

MoonBall commented Apr 10, 2018 • edited Loading

MoonBall Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Flarna Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

MoonBall Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Flarna Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Trott Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Flarna commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 12, 2018

trivikr commented Apr 12, 2018

BridgeAR commented Apr 13, 2018

Flarna commented Apr 9, 2018 •

edited by BridgeAR

Loading

Trott Apr 9, 2018 •

edited

Loading

Trott Apr 9, 2018 •

edited

Loading

MoonBall commented Apr 10, 2018 •

edited

Loading

MoonBall Apr 10, 2018 •

edited

Loading

Flarna Apr 10, 2018 •

edited

Loading

MoonBall Apr 10, 2018 •

edited

Loading

Flarna Apr 10, 2018 •

edited

Loading

Trott Apr 10, 2018 •

edited

Loading