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.

Sign up for GitHub

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

IncommingMessage extends Readable, but it's destroy method not return this #32772

Closed
LongTengDao opened this issue Apr 10, 2020 · 0 comments · Fixed by #32789
Closed

IncommingMessage extends Readable, but it's destroy method not return this #32772

LongTengDao opened this issue Apr 10, 2020 · 0 comments · Fixed by #32789

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented Apr 10, 2020 •
edited
Loading

Is your feature request related to a problem? Please describe.

DefinitelyTyped/DefinitelyTyped#43782

node doc said stream.destroy should return this, but in old node.d.ts, it was wrongly written void.

when I want to fix this, I found I can't pass the test, because IncommingMessage extends stream, and its destroy method return void.

when I check soure code for node, I found in fact IncommingMessage.prototype.destroy return void, here it's not wrong.

but they are conflict, and I can't fix them togethor only in node.d.ts.

Describe the solution you'd like

make IncommingMessage.prototype.destroy return this

Describe alternatives you've considered

make Readable.prototype.destroy and Writable.prototype.destroy return void

I'm not sure which will make a bigger break change, I think we must discuss carefully
@cjihrig cjihrig mentioned this issue Apr 11, 2020
Merged
3 tasks
@cjihrig cjihrig closed this as completed in ff6535a May 9, 2020
codebytere pushed a commit that referenced this issue Jun 27, 2020
@cjihrig @codebytere
http: return this from IncomingMessage#destroy()
c795955 
This commit updates IncomingMessage#destroy() to return `this`
for consistency with other readable streams.

PR-URL: #32789
Fixes: #32772
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this issue Jun 30, 2020
@cjihrig @codebytere
http: return this from IncomingMessage#destroy()
28dcfd1 
This commit updates IncomingMessage#destroy() to return `this`
for consistency with other readable streams.

PR-URL: #32789
Fixes: #32772
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit that referenced this issue Sep 22, 2020
@cjihrig @addaleax
http: return this from IncomingMessage#destroy()
fb68487 
This commit updates IncomingMessage#destroy() to return `this`
for consistency with other readable streams.

PR-URL: #32789
Fixes: #32772
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http: return this from IncomingMessage#destroy() cjihrig/node
1 participant
@LongTengDao