Skip to content

doc: improve http.setHeader and getHeader typeinfo #19902

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

Closed
Flarna wants to merge 5 commits into from
Closed

doc: improve http.setHeader and getHeader typeinfo #19902

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3699198
doc: improve http.setHeader and getHeader typeinfo
Flarna Apr 9, 2018
32685cf
fix review items
Flarna Apr 10, 2018
1d5c3e2
fix review items
Flarna Apr 10, 2018
5dc2a5d
fix review findings, correct request and response APIs
Flarna Apr 10, 2018
3ab5dc7
setHeader accepts type any
Flarna Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions doc/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,13 +587,21 @@ added: v1.6.0
-->

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

@MoonBall MoonBall Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be * Returns: {any}? Because

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

@Flarna Flarna Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.


Reads out a header on the request. Note that the name is case insensitive.
The type of the return value depends on the arguments provided to
[`response.setHeader()`][].

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

@vsemozhetbyt vsemozhetbyt Apr 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

: -> ,

request.setHeader('Set-Cookie', ['type=ninja', 'language=javascript'])

@vsemozhetbyt vsemozhetbyt Apr 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing semicolon.

const contentType = request.getHeader('Content-Type'); // contentType is 'text/html'

@vsemozhetbyt vsemozhetbyt Apr 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

const contentLength = request.getHeader('Content-Length'); // contentLength is of type number

@vsemozhetbyt vsemozhetbyt Apr 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Flarna Flarna Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@vsemozhetbyt vsemozhetbyt Apr 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Trott Trott Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

const setCookie = request.getHeader('set-cookie'); // setCookie is of type string[]

```

### request.removeHeader(name)
Expand Down Expand Up @@ -1210,7 +1218,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.

@MoonBall MoonBall Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@Flarna Flarna Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@Trott Trott Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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)?


Example:

Expand Down