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

doc: clarify IncomingMessage is a new Readable "forked" from the socket by dr-js · Pull Request #36641 · nodejs/node · GitHub

/ node Public

doc: clarify IncomingMessage is a new Readable "forked" from the socket - #36641

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
dr-js:patch-1
Dec 30, 2020
ConversationCommits (1)ChecksFiles changed
Merged

doc: clarify IncomingMessage is a new Readable "forked" from the socket#36641
aduh95 merged 1 commit into
nodejs:masterfrom
dr-js:patch-1

Conversation

dr-js commented Dec 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Refs: #36617

Added more description on the difference between the IncomingMessage and the underlying message.socket.

Added a separate commit to mark the change of v15.5.0 and deprecate message.aborted per #36617 (comment). (moved to #36670)

Checklist

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Dec 27, 2020
dr-js mentioned this pull request Dec 27, 2020
Closed
targos reviewed Dec 27, 2020
View reviewed changes
Comment thread doc/api/http.md Outdated
aduh95 added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 27, 2020
RaisinTen reviewed Dec 27, 2020
View reviewed changes
Comment thread doc/api/http.md Outdated
aduh95 requested changes Dec 27, 2020
View reviewed changes

aduh95 left a comment

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

This should add the deprecation info to doc/api/deprecations.md as well.

targos commented Dec 27, 2020

Copy link
Copy Markdown
Member

Do we have a policy to treat doc-only deprecations as semver-minor?

aduh95 removed the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 27, 2020
dr-js force-pushed the patch-1 branch 2 times, most recently from 76a9da1 to 8bcc099 Compare December 28, 2020 00:51
aduh95 reviewed Dec 28, 2020
View reviewed changes
Comment thread doc/api/deprecations.md Outdated

dr-js commented Dec 28, 2020

Copy link
Copy Markdown
Contributor Author

@aduh95 I wrote some test to compare current message.aborted behavior with message.destroyed at test-nodejs-message-aborted.js, and here is what I think:

Only directly call .abort() will emit the 'abort' event and set .aborted
to true, the .destroyed attribute will be set to true at the same time,
and the Stream 'error' and 'close' event will follow. Calling the Stream
.destroy() will emit only the 'close' event, and may not set the .aborted
to true.

So the abort related value and event is only useful for detecting .abort()
calls. For closing a request early, call the Stream .destroy() and listen for
'close' and 'error' should have the same effect and wider code support.

I'll add the second paragraph as the explanation.

Do we doc-deprecate event?
As currently request.abort() is marked, but the 'abort' event is not, so is the request.aborted kept.

Also it's better if @ronag could review the added explanation.

ronag commented Dec 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

I think deprecation is something for a separate PR. I would personally be happy to doc deprecate:

  • 'abort' event.
  • 'aborted' event.
  • .aborted property.
  • .abort() function.

dr-js commented Dec 29, 2020

Copy link
Copy Markdown
Contributor Author

@ronag then I'll split deprecation related commit to a separate PR and link back

dr-js mentioned this pull request Dec 29, 2020
Closed
2 tasks
dr-js requested a review from aduh95 December 30, 2020 11:53
aduh95 removed the notable-change PRs with changes that should be highlighted in changelogs. label Dec 30, 2020
doc: document http.IncomingMessage behaviour change
a5b43e9 
Add to the history table that the `destroyed` value returns `true` after
the incoming data is consumed.

Refs: nodejs#36617
Refs: nodejs#33035

PR-URL: nodejs#36641
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
aduh95 self-assigned this Dec 30, 2020

aduh95 commented Dec 30, 2020

Copy link
Copy Markdown
Contributor

Landed in a5b43e9

aduh95 merged commit a5b43e9 into nodejs:master Dec 30, 2020
dr-js deleted the patch-1 branch December 30, 2020 15:39
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
doc: document http.IncomingMessage behaviour change
651e7d2 
Add to the history table that the `destroyed` value returns `true` after
the incoming data is consumed.

Refs: #36617
Refs: #33035

PR-URL: #36641
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
danielleadams mentioned this pull request Jan 12, 2021
Merged
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
doc: deprecate (doc-only) http abort related
d9fb8ae 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
mcollina pushed a commit that referenced this pull request Sep 27, 2021
doc: deprecate (doc-only) http abort related
4b030d0 
Refs: #36641
Refs: #36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Oct 5, 2021
doc: deprecate (doc-only) http abort related
8c76fa5 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Oct 9, 2021
doc: deprecate (doc-only) http abort related
4da1318 
Refs: #36641
Refs: #36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 12, 2021
doc: deprecate (doc-only) http abort related
d1d9f2d 
Refs: #36641
Refs: #36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

RaisinTen RaisinTen left review comments
ronag ronag approved these changes
aduh95 aduh95 approved these changes
+1 more reviewer
targos targos left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

aduh95

Labels

doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL