doc: clearify that http does chunked encoding itself. - #28379
Conversation
commented
Jun 22, 2019
|
Right… The commit guidelines didn't say no punctuation. I guess anyone merging this can simply do a --amend. |
left a comment
There was a problem hiding this comment.
Welcome @micromaomao and thanks for the pull request! As it currently stands, I think this addition to the docs probably creates more confusion than it clarifies. From the text, it's not clear if chunked is the default for the optional encoding argument, or if chunked encoding is a thing that it always does but that's separate/different from the encoding argument supplied to the function.
There was a problem hiding this comment.
| Note that this module automatically encode data into chunked encoding. | |
| This module automatically encodes data in chunked encoding. |
(That's a grammar fix and a style fix, but the content is still problematic, I think.)
|
I see what you're saying. I also noticed another issue, the doc already says:
What does suggested to use mean? According to my very quick scan of the source code in That aside, I think maybe the sentence should be rephrased as something like: This method can be called multiple times. If no Please correct me if my understanding is incorrect. |
commented
Jun 22, 2019
In general, I'd be inclined to simply replace any occurrences of it is suggested to use or it is recommended to use to the more succinct use. In this particular case, if the suggestion is unnecessary, I'd remove it entirely.
/ping @nodejs/documentation @nodejs/http Does that change seem correct to you? (There are a few minor style changes I'd make to that, but I'm mostly concerned about getting the content and meaning correct first.) |
commented
Jul 4, 2019
|
@nodejs/http @nodejs/http2 PTAL |
left a comment
There was a problem hiding this comment.
I'm good with an edited version of the text in #28379 (comment).
Would you mind adding it to the PR?
|
@mcollina Updated. @Trott wanted some style change. Also, looking at it now, I can see how the change may make the part that says
a little bit confusing. Maybe we should make it clear that the |
There was a problem hiding this comment.
If no
Content-Lengthis set, data will automatically be encoded in HTTP
Chunked transfer encoding, so that server knows when data ends. TheTransfer-Encoding: chunkedheader will be automatically added.
Would be simpler maybe?
There was a problem hiding this comment.
With a reference to https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback? Even though it is not a Writable the intention is to act as similar as possible. @mcollina, what do you think? Would such a reference be harmful?
commented
Apr 1, 2020
|
@micromaomao, can you follow up with review comments ? and this needs a rebase. |
8ae28ff to
2935f72
Compare
commented
Oct 19, 2020
|
@nodejs/http This needs reviews. |
There was a problem hiding this comment.
I would retain the first sentence
Sends a chunk of the body.
commented
Jan 9, 2021
|
@micromaomao Can you address the last comment please? |
commented
Jan 9, 2021
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
commented
Jan 9, 2021
|
I think it's alright. Do you want me to amend the commit adding the "Sends a chunk of the body." sentence to the beginning? |
commented
Jan 11, 2021
|
amended, squashed and rebase'd. Also it's been one year so someone probably needs to verify that this is still the behavior of the function. |
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
commented
Jan 11, 2021
|
@aduh95 done |
commented
Jun 13, 2021
|
@nodejs/http Can we land this? |
commented
Jun 13, 2021
commented
Jun 13, 2021
|
Landed in 67d4a3f |
I don't know if I'm putting it in the right place in the doc (or if there is a better way to present this information), but I think stating this is necessary, and I couldn't find any mention of the fact that chunked encoding is handled by node itself in the existing http doc.
Checklist