FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: fix misleading sentence in http.md by lpinca · Pull Request #26465 · nodejs/node · GitHub

/ node Public

doc: fix misleading sentence in http.md - #26465

Closed
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:gh-26005
Closed

doc: fix misleading sentence in http.md#26465
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:gh-26005

Conversation

lpinca commented Mar 6, 2019

Copy link
Copy Markdown
Member

Calling response.end(data) is not 100% equivalent to calling
response.write(data) followed by response.end().

Fixes: #26005

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Copy link
Copy Markdown
Collaborator

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Mar 6, 2019
BridgeAR previously approved these changes Mar 6, 2019
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 6, 2019
Comment thread doc/api/http.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit:

Suggested change
`chunked`, otherwise the `Transfer-Encoding` header is replaced by the
`'chunked'`, otherwise the `Transfer-Encoding` header is replaced by the

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Optional suggestion: Change the comma to a period and make the Otherwise clause its own sentence.

Copy link
Copy Markdown
Member

Commit message has misspelled sentence.

Comment thread doc/api/http.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

lpinca Mar 6, 2019
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

nit: I think a commit message of doc: describe when chunked mode is used would be more descriptive of the specific change being made.

Calling `response.end(data)` is not 100% equivalent to calling
`response.write(data)` followed by `response.end()`.

Fixes: nodejs#26005

lpinca commented Mar 7, 2019

Copy link
Copy Markdown
Member Author

Folks that approved this, PTAL, I've updated as per @sam-github suggestion.

BridgeAR dismissed their stale review March 7, 2019 08:47

Abstain

BridgeAR commented Mar 7, 2019

Copy link
Copy Markdown
Member

I would rather have a detailed information but I won't block this either.

lpinca commented Mar 7, 2019
edited
Loading

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

What should be done here?

@jasnell PTAL

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 13, 2019
Calling `response.end(data)` is not 100% equivalent to calling
`response.write(data)` followed by `response.end()`.

PR-URL: nodejs#26465
Fixes: nodejs#26005
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Copy link
Copy Markdown
Member

Landed in 1706a2d 🎉

BridgeAR closed this Mar 13, 2019
lpinca deleted the gh-26005 branch March 13, 2019 15:47
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
Calling `response.end(data)` is not 100% equivalent to calling
`response.write(data)` followed by `response.end()`.

PR-URL: #26465
Fixes: #26005
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 14, 2019
Calling `response.end(data)` is not 100% equivalent to calling
`response.write(data)` followed by `response.end()`.

PR-URL: nodejs#26465
Fixes: nodejs#26005
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
Calling `response.end(data)` is not 100% equivalent to calling
`response.write(data)` followed by `response.end()`.

PR-URL: #26465
Fixes: #26005
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

doc(http): res.write(data) followed by res.end() is not actually equivalent to res.end(data)

9 participants


Back | FazBrowse Home | New Git URL