stream, http, http2: make all stream.end() call return this #18780
Conversation
nodejs-github-bot
added
the
lib / src
|
cc @mafintosh @nodejs/streams |
|
Can this change in return type really be justified? Also:
|
benjamingr
added
the
semver-major
|
@mcollina I'm guessing we missed some context - any reading material about this change or some rationale? |
|
Adding a return type where there was previously not one should not be semver-major as it shouldn't break any prior assumptions. |
|
@jasnell uhh.. I think it can break assumptions? I've seen |
|
Bleh, ok. Major it is then. |
|
I forgot to put the label on it, the intention is it to be semver-major. Basically this cleans up the situation for .end() in streams. Currently we have net and tls that return this, the stream module that returns undefined, and http that return whatever _send() returns. I thought that the least disruptive change would be to have it return this everywhere. |
doc/api/http.md
Outdated
| changes: | ||
| - version: REPLACEME | ||
| pr-url: https://github.com/nodejs/node/pull/18780 | ||
| description: This method now returns a reference to `http.ClientRequest`. |
There was a problem hiding this comment.
Nit: how about using just ClientRequest to be consistent with the other comments?
BridgeAR
added
the
author ready
|
@mscdex are you ok with this change then? |
PR-URL: #18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
|
Awesome work, adding "don't land on" tags. |
PR-URL: nodejs#18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This PR make all the instances of
stream.end()return this. This includes both http and http2 pseudo-writables.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream, http, http2