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

doc: remove warning from response.writeHead #32700

Closed
wants to merge 2 commits into from
Closed

Conversation

cecchi
Copy link
Contributor

@cecchi cecchi commented Apr 7, 2020

The example referenced as being potentially unsafe specifies Content-Length correctly.

The current documentation warns that the example preceding this note only works because the characters in "hello world" are all single-byte characters. The example however already uses Buffer.byteLength, which is the recommended "fix".

Checklist

The example referenced as being potentially unsafe specifies
Content-Length correctly.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Apr 7, 2020
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

Maybe it still makes sense to leave a references (and ideally a link) to Buffer.byteLength() there? It can be short, e.g. “Use Buffer.byteLength() to determine the length of the body in bytes.”

@cecchi
Copy link
Contributor Author

cecchi commented Apr 7, 2020

@addaleax sounds good. Updated with that language & link.

Flarna pushed a commit that referenced this pull request Apr 13, 2020
The example referenced as being potentially unsafe specifies
Content-Length correctly.

PR-URL: #32700
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@Flarna
Copy link
Member

Flarna commented Apr 13, 2020

Landed in 61e589f

@Flarna Flarna closed this Apr 13, 2020
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
The example referenced as being potentially unsafe specifies
Content-Length correctly.

PR-URL: #32700
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
The example referenced as being potentially unsafe specifies
Content-Length correctly.

PR-URL: nodejs#32700
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
The example referenced as being potentially unsafe specifies
Content-Length correctly.

PR-URL: #32700
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
The example referenced as being potentially unsafe specifies
Content-Length correctly.

PR-URL: #32700
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants