Skip to content

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

Closed
micromaomao wants to merge 1 commit into
nodejs:masterfrom
micromaomao:patch-1
Closed

doc: clearify that http does chunked encoding itself.#28379
micromaomao wants to merge 1 commit into
nodejs:masterfrom
micromaomao:patch-1

Conversation

@micromaomao

@micromaomao micromaomao commented Jun 22, 2019

Copy link
Copy Markdown
Contributor

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. http Issues and PRs related to the http subsystem. labels Jun 22, 2019
@micromaomao

ghost commented Jun 22, 2019

Copy link
Copy Markdown
Contributor Author

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

I guess anyone merging this can simply do a --amend.

Trott
Trott previously requested changes Jun 22, 2019

ghost left a comment

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.

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.

Comment thread doc/api/http.md Outdated

ghost Jun 22, 2019

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.

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

ghost commented Jun 22, 2019

Copy link
Copy Markdown
Contributor Author

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

ghost commented Jun 22, 2019

Copy link
Copy Markdown
Member

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.)

@jasnell jasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jun 23, 2019
@addaleax addaleax removed the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 3, 2019
@BridgeAR

ghost commented Jul 4, 2019

Copy link
Copy Markdown
Member

@nodejs/http @nodejs/http2 PTAL

ghost left a comment

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.

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

Would you mind adding it to the PR?

@micromaomao

ghost commented Jul 5, 2019

Copy link
Copy Markdown
Contributor Author

@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?

Comment thread doc/api/http.md Outdated

ghost Mar 11, 2020

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.

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?

ghost Mar 11, 2020

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.

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

ghost commented Apr 1, 2020

Copy link
Copy Markdown
Member

@micromaomao, can you follow up with review comments ? and this needs a rebase.

@BridgeAR
BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Jul 3, 2020
@micromaomao
micromaomao requested a review from a team as a code owner August 10, 2020 16:08
@aduh95 aduh95 added the review wanted PRs that need review. label Oct 19, 2020
@aduh95

ghost commented Oct 19, 2020

Copy link
Copy Markdown
Contributor

@nodejs/http This needs reviews.

Comment thread doc/api/http.md Outdated

ghost Oct 19, 2020

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.

I would retain the first sentence

Sends a chunk of the body.

@aduh95

ghost commented Jan 9, 2021

Copy link
Copy Markdown
Contributor

@micromaomao Can you address the last comment please?

@aduh95 aduh95 added stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. and removed stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. labels Jan 9, 2021
@github-actions

ghost commented Jan 9, 2021

Copy link
Copy Markdown
Contributor

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

ghost commented Jan 9, 2021

Copy link
Copy Markdown
Contributor Author

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

ghost commented Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

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.

Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@micromaomao

ghost commented Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

@aduh95 done

@aduh95
aduh95 requested review from mcollina and removed request for a team January 11, 2021 13:27
@aduh95

ghost commented Jun 13, 2021

Copy link
Copy Markdown
Contributor

@nodejs/http Can we land this?

@nodejs-github-bot

ghost commented Jun 13, 2021

Copy link
Copy Markdown
Collaborator

ghost left a comment

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.

lgtm

@dnlup

ghost commented Jun 13, 2021

Copy link
Copy Markdown
Contributor

Landed in 67d4a3f

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 Node.js documentation. http Issues and PRs related to the http subsystem. review wanted PRs that need review. stalled Issues and PRs manually marked as stalled and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.