Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign updoc: fix misleading sentence in http.md #26465
Conversation
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
doc
http
labels
Mar 6, 2019
BridgeAR
reviewed
Mar 6, 2019
BridgeAR
added
the
author ready
label
Mar 6, 2019
ZYSzys
approved these changes
Mar 6, 2019
vsemozhetbyt
reviewed
Mar 6, 2019
| [`response.write(data, encoding)`][] followed by `response.end(callback)`. There | ||
| is, however, a difference in the headers that are sent by default. If | ||
| [`response.write()`][] is used, the `Transfer-Encoding` header is set to | ||
| `chunked`, otherwise the `Transfer-Encoding` header is replaced by the |
This comment has been minimized.
This comment has been minimized.
vsemozhetbyt
Mar 6, 2019
Member
Nit:
| `chunked`, otherwise the `Transfer-Encoding` header is replaced by the | |
| `'chunked'`, otherwise the `Transfer-Encoding` header is replaced by the |
This comment has been minimized.
This comment has been minimized.
Trott
Mar 6, 2019
Member
Optional suggestion: Change the comma to a period and make the Otherwise clause its own sentence.
This comment has been minimized.
This comment has been minimized.
|
Commit message has misspelled |
lpinca
force-pushed the
lpinca:gh-26005
branch
from
ebb49fa
to
286f7ab
Mar 6, 2019
sam-github
reviewed
Mar 6, 2019
| is, however, a difference in the headers that are sent by default. If | ||
| [`response.write()`][] is used, the `Transfer-Encoding` header is set to | ||
| `'chunked'`. Otherwise the `Transfer-Encoding` header is replaced by the | ||
| `Content-Length` header. |
This comment has been minimized.
This comment has been minimized.
sam-github
Mar 6, 2019
Member
I think this leaves one case undescribed: response.write(data); response.end(data)
Also, while accurate, so an improvement, I think its too low level, it doesn't give any idea why its done this way. And if you knew why, then you wouldn't need the docs.
I'd suggest that the .write() docs be modified to say that because .write() can be called multiple times, the size of the entire response (or request, I suspect these docs apply to both req and response) cannot be known when .write() is called, so HTTP will use chunked transfer encoding, and there will not be a content-size header.
In .end(), I'd say that if .write() has not previously been called, the total size is known (zero, or the size of the data arg), so node.js doesn't have to (and won't) use chunked transfer encoding, and it will write a content-length header. In other cases, since chunked transfer has already begun, any data provided to .end() will be transferred as a chunk.
^--- I think this is accurate, and describes why this is, and makes this seem less like a weird gotcha, and more like a feature: that if you have one large chunk that can be written in a single call to .end(), you can cause the content-size to be set and chunked mode not to be used. I'm not sure if it applies equally to the writing of requests, and the writing of responponses... but it should!
This comment has been minimized.
This comment has been minimized.
lpinca
Mar 6, 2019
•
Author
Member
I only wanted to address the misleading sentence quoted in #26005 and not describe chunk mode. This is also not 100% accurate as the user could explicitly specify Content-Length disabling the default chunked mode. Perhaps it's simpler to just remove that sentence?
This comment has been minimized.
This comment has been minimized.
sam-github
Mar 6, 2019
Member
It would be nice to have the info about how explicitly setting the header changes things. Basically, that Node.js has good support for chunked mode (unlike a number of crappy HTTP APIs I've come across), is a wonderful feature, its a shame we don't document it better.
Your comment above makes me wonder if your proposed text is accurate/complete, doesn't it suffer the same problem?
For now, yes, I think rather than dumping in the detail you proposed, if you aren't up to a really comprehensive treatment, just changing "the same as" to "similar in effect to", or similar weasel words, might make the docs "not wrong", even though it still leaves streaming vs non-streaming mode undoced.
This comment has been minimized.
This comment has been minimized.
lpinca
Mar 6, 2019
Author
Member
Your comment above makes me wonder if your proposed text is accurate/complete, doesn't it suffer the same problem?
Yes, I've added "the headers that are sent by default" to address that but I guess it's not clear.
I will change this tomorrow and use "similar" removing all the chunked mode info.
FWIW the request counterparts are slightly better documented.
sam-github
changed the title
doc: fix misleading sentece in http.md
doc: fix misleading sentence in http.md
Mar 6, 2019
This comment has been minimized.
This comment has been minimized.
|
nit: I think a commit message of |
jasnell
approved these changes
Mar 7, 2019
lpinca
force-pushed the
lpinca:gh-26005
branch
from
286f7ab
to
f80563f
Mar 7, 2019
This comment has been minimized.
This comment has been minimized.
|
Folks that approved this, PTAL, I've updated as per @sam-github suggestion. |
BridgeAR
dismissed
their
stale review
Mar 7, 2019
Abstain
This comment has been minimized.
This comment has been minimized.
|
I would rather have a detailed information but I won't block this either. |
This comment has been minimized.
This comment has been minimized.
|
It makes sense but I don't have the energy to update the docs as suggested in #26465 (comment) explaining how and when chunked mode is used, how to disable it, etc. especially due to all the nit picking that will come from that. If that is preferred I would prefer it to be done by a native speaker. |
This comment has been minimized.
This comment has been minimized.
|
What should be done here? @jasnell PTAL |
sam-github
approved these changes
Mar 13, 2019
added a commit
to BridgeAR/node
that referenced
this pull request
Mar 13, 2019
This comment has been minimized.
This comment has been minimized.
|
Landed in 1706a2d |
lpinca commentedMar 6, 2019
Calling
response.end(data)is not 100% equivalent to callingresponse.write(data)followed byresponse.end().Fixes: #26005
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes