X Tutup
The Wayback Machine - https://web.archive.org/web/20210814084256/https://github.com/nodejs/node/pull/28379
Skip to content

/ node

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

Jump to bottom

doc: clearify that http does chunked encoding itself. #28379

Closed
wants to merge 1 commit into from
Closed

doc: clearify that http does chunked encoding itself. #28379

wants to merge 1 commit into from

Conversation

@micromaomao
Copy link
Contributor

@micromaomao micromaomao commented Jun 22, 2019 •
edited

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
@micromaomao
Copy link
Contributor Author

@micromaomao micromaomao commented Jun 22, 2019

Right… The commit guidelines didn't say no punctuation.

I guess anyone merging this can simply do a --amend.
@Trott
Trott reviewed Jun 22, 2019
View changes
Copy link
Member

@Trott Trott left a 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.
doc/api/http.md Outdated
@@ -772,6 +772,8 @@ server — in that case it is suggested to use the
`['Transfer-Encoding', 'chunked']` header line when
creating the request.

Note that this module automatically encode data into chunked encoding.

This comment has been minimized.

Sign in to view
@Trott

Trott Jun 22, 2019
Member

Suggested change
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.)
@micromaomao
Copy link
Contributor Author

@micromaomao micromaomao commented Jun 22, 2019 •
edited

I see what you're saying.

I also noticed another issue, the doc already says:

By calling this method many times, a request body can be sent to a server — in that case it is suggested to use the ['Transfer-Encoding', 'chunked'] header line when creating the request.

What does suggested to use mean? According to my very quick scan of the source code in lib/_http_outgoing.js:360 (this is my first time reading node source, so I might have looked in the wrong place), the Transfer-Encoding header is automatically added if Content-Length is not provided, and node will automatically encode data with chunked encoding. So I think it isn't clear what is the difference if the user add the Transfer-Encoding: chunked header or not.

That aside, I think maybe the sentence should be rephrased as something like:

This method can be called multiple times. If no Content-Length is set, data will automatically be encoded in HTTP Chunked transfer encoding, so that server knows when data ends. The Transfer-Encoding: chunked header will be added. Call end() when finish sending data so that node would send the trailer.

Please correct me if my understanding is incorrect.
@Trott
Copy link
Member

@Trott Trott commented Jun 22, 2019

What does suggested to use mean?

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.

That aside, I think maybe the sentence should be rephrased as something like:

This method can be called multiple times. If no Content-Length is set, data will automatically be encoded in HTTP Chunked transfer encoding, so that server knows when data ends. The Transfer-Encoding: chunked header will be added. Call end() when finish sending data so that node would send the trailer.

/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.)
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 4, 2019

@nodejs/http @nodejs/http2 PTAL
@mcollina
mcollina requested changes Jul 5, 2019
View changes
Copy link
Member

@mcollina mcollina left a comment

I'm good with an edited version of the text in #28379 (comment).

Would you mind adding it to the PR?
@micromaomao
Copy link
Contributor Author

@micromaomao micromaomao commented Jul 5, 2019 •
edited

@mcollina Updated.

@Trott wanted some style change. Also, looking at it now, I can see how the change may make the part that says

The encoding argument is optional and only applies when chunk is a string. Defaults to 'utf8'.

a little bit confusing. Maybe we should make it clear that the encoding argument has nothing to do with the chunked transfer encoding, by e.g. saying that it is about encoding the string to Buffer?
@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
@devnexen devnexen force-pushed the nodejs:master branch from e8a4568 to 5289f80 Dec 26, 2019
@BridgeAR BridgeAR requested review from mcollina and Trott Jan 12, 2020
@Trott Trott dismissed their stale review Jan 26, 2020

style changes applied

@ronag
ronag reviewed Mar 11, 2020
View changes
doc/api/http.md Outdated
will automatically be encoded in HTTP Chunked transfer encoding, so that
server knows when data ends. The `Transfer-Encoding: chunked` header will be
added. Call `end()` when finish sending data so that node would send the
trailer.

This comment has been minimized.

Sign in to view
@ronag

ronag Mar 11, 2020
Member

If no Content-Length is set, data will automatically be encoded in HTTP
Chunked transfer encoding, so that server knows when data ends. The Transfer-Encoding: chunked header will be automatically added.

Would be simpler maybe?

This comment has been minimized.

Sign in to view
@ronag

ronag Mar 11, 2020
Member

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?
@HarshithaKP
Copy link
Member

@HarshithaKP HarshithaKP commented Apr 1, 2020

@micromaomao, can you follow up with review comments ? and this needs a rebase.
@micromaomao micromaomao force-pushed the micromaomao:patch-1 branch to 1bb99bb Apr 11, 2020
@BridgeAR BridgeAR force-pushed the nodejs:master branch 2 times, most recently from 8ae28ff to 2935f72 May 31, 2020
@jasnell jasnell added the stalled label Jul 3, 2020
@micromaomao micromaomao requested a review from nodejs/net as a code owner Aug 10, 2020
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 19, 2020

@nodejs/http This needs reviews.
@mcollina
mcollina reviewed Oct 19, 2020
View changes
doc/api/http.md Outdated
server. In that case, it is suggested to use the
`['Transfer-Encoding', 'chunked']` header line when
creating the request.
This method can be called multiple times. If no `Content-Length` is set, data

This comment has been minimized.

Sign in to view
@mcollina

mcollina Oct 19, 2020
Member

I would retain the first sentence

Sends a chunk of the body.
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 9, 2021

@micromaomao Can you address the last comment please?
@aduh95 aduh95 added stalled and removed stalled labels Jan 9, 2021
@github-actions
Copy link

@github-actions github-actions bot 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.
@micromaomao
Copy link
Contributor Author

@micromaomao micromaomao 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?
@micromaomao micromaomao force-pushed the micromaomao:patch-1 branch from 1bb99bb Jan 11, 2021
@micromaomao
Copy link
Contributor Author

@micromaomao micromaomao 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.
@aduh95
aduh95 reviewed Jan 11, 2021
View changes
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
@micromaomao micromaomao force-pushed the micromaomao:patch-1 branch Jan 11, 2021
@micromaomao @aduh95
doc: clearify that http does chunked encoding itself
f3874db 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@micromaomao micromaomao force-pushed the micromaomao:patch-1 branch to f3874db Jan 11, 2021
@micromaomao
Copy link
Contributor Author

@micromaomao micromaomao commented Jan 11, 2021

@aduh95 done
@aduh95
aduh95 approved these changes Jan 11, 2021
View changes
@aduh95 aduh95 requested review from mcollina and removed request for nodejs/net Jan 11, 2021
@guybedford guybedford force-pushed the nodejs:master branch from dc5a5da to 8e46568 Mar 29, 2021
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 13, 2021

@nodejs/http Can we land this?
@addaleax
addaleax approved these changes Jun 13, 2021
View changes
@dnlup
dnlup approved these changes Jun 13, 2021
View changes
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Jun 13, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38616/
@mcollina
mcollina approved these changes Jun 13, 2021
View changes
Copy link
Member

@mcollina mcollina left a comment

lgtm
dnlup added a commit that referenced this pull request Jun 13, 2021
@micromaomao @dnlup
doc: clearify that http does chunked encoding itself
67d4a3f 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #28379
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@dnlup
Copy link
Member

@dnlup dnlup commented Jun 13, 2021

Landed in 67d4a3f
@dnlup dnlup closed this Jun 13, 2021
danielleadams added a commit that referenced this pull request Jun 14, 2021
@micromaomao @danielleadams
doc: clearify that http does chunked encoding itself
ed91379 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #28379
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
danielleadams added a commit that referenced this pull request Jun 17, 2021
@micromaomao @danielleadams
doc: clearify that http does chunked encoding itself
21d6ebc 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #28379
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag

@mcollina mcollina

@addaleax addaleax

@aduh95 aduh95

@dnlup dnlup

@Trott Trott

Assignees
No one assigned
Labels
doc http review wanted stalled
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

X Tutup