stream, http, http2: make all stream.end() call return this by mcollina · Pull Request #18780 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits3 Checks0 Files changed

Conversation

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]({{ revealButtonHref }})

This PR make all the instances of stream.end() return this. This includes both http and http2 pseudo-writables.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

stream, http, http2

Can this change in return type really be justified?

Also:

an early return is missed in OutgoingMessage.end()
shouldn't this be semver-major because of the change in return type?

benjamingr added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Feb 14, 2018

@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 return somethingThatReturnsUndefined() in JavaScript quite a few times in order to avoid doing somethingThatReturnsUndefined(); return; in two lines. I don't like it, but it's definitely there - and this isn't a change in an experimental library - it's core HTTP which everyone builds on.

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.

Choose a reason for hiding this comment

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

Thanks, that explains it and this does bring consistency. Actual code LGTM

changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/18780
description: This method now returns a reference to `http.ClientRequest`.

Choose a reason for hiding this comment

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

Nit: how about using just ClientRequest to be consistent with the other comments?

Choose a reason for hiding this comment

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

Done.

BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Feb 16, 2018

@mscdex are you ok with this change then?

Landed as f6721c2, 8118da7 and 3d93f39.

mcollina added a commit that referenced this pull request

Feb 19, 2018

PR-URL: #18780 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Luigi Pinca luigipinca@gmail.com

mcollina added a commit that referenced this pull request

Feb 19, 2018

mcollina added a commit that referenced this pull request

Feb 19, 2018