Skip to content
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

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

Merged
merged 1 commit into from Dec 30, 2020

Conversation

dr-js
Copy link
Contributor

@dr-js dr-js commented Dec 27, 2020

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 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
doc/api/http.md Outdated Show resolved Hide resolved
@aduh95 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
doc/api/http.md Outdated Show resolved Hide resolved
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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

@targos
Copy link
Member

targos commented Dec 27, 2020

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

@aduh95 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 dr-js force-pushed the patch-1 branch 2 times, most recently from 76a9da1 to 8bcc099 Compare December 28, 2020 00:51
doc/api/deprecations.md Outdated Show resolved Hide resolved
@dr-js
Copy link
Contributor Author

dr-js commented Dec 28, 2020

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

ronag commented Dec 28, 2020

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

dr-js commented Dec 29, 2020

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

dr-js added a commit to dr-js/node that referenced this pull request Dec 29, 2020
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>
@dr-js dr-js requested a review from aduh95 December 30, 2020 11:53
@aduh95 aduh95 removed the notable-change PRs with changes that should be highlighted in changelogs. label Dec 30, 2020
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
Copy link
Contributor

aduh95 commented Dec 30, 2020

Landed in a5b43e9

@aduh95 aduh95 merged commit a5b43e9 into nodejs:master Dec 30, 2020
@dr-js dr-js deleted the patch-1 branch December 30, 2020 15:39
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
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 danielleadams mentioned this pull request Jan 12, 2021
dr-js added a commit to dr-js/node that referenced this pull request Jan 19, 2021
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>
dr-js added a commit to dr-js/node that referenced this pull request Feb 3, 2021
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>
dr-js added a commit to dr-js/node that referenced this pull request Mar 4, 2021
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>
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
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>
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
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>
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
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
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
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>
dr-js added a commit to dr-js/node that referenced this pull request Oct 5, 2021
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
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
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>
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 the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants