Skip to content

/node

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

stream, http, http2: make all stream.end() call return this #18780

Closed
wants to merge 3 commits into from

Conversation

Reviewers

@jasnell jasnell

@addaleax addaleax

@benjamingr benjamingr

@lpinca lpinca

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
author ready dont-land-on-v6.x dont-land-on-v8.x lib / src semver-major
Projects
None yet
Milestone
No milestone
8 participants
@mcollina @mscdex @benjamingr @jasnell @BridgeAR @addaleax @lpinca @nodejs-github-bot
@mcollina
Copy link
Member

mcollina commented Feb 14, 2018

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

@mcollina mcollina requested a review from addaleax Feb 14, 2018

@nodejs-github-bot nodejs-github-bot added the lib / src label Feb 14, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Feb 14, 2018

cc @mafintosh @nodejs/streams
@mscdex

This comment has been minimized.

Sign in to view
Copy link
Contributor

mscdex commented Feb 14, 2018

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 benjamingr added the semver-major label Feb 14, 2018

@benjamingr

This comment has been minimized.

Sign in to view
Copy link
Member

benjamingr commented Feb 14, 2018

@mcollina I'm guessing we missed some context - any reading material about this change or some rationale?
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Feb 14, 2018

Adding a return type where there was previously not one should not be semver-major as it shouldn't break any prior assumptions.
@jasnell

jasnell approved these changes Feb 14, 2018
View changes

@benjamingr

This comment has been minimized.

Sign in to view
Copy link
Member

benjamingr commented Feb 14, 2018

@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.
@BridgeAR

BridgeAR approved these changes Feb 14, 2018
View changes

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Feb 14, 2018

Bleh, ok. Major it is then.
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Feb 14, 2018

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.
@benjamingr

benjamingr approved these changes Feb 14, 2018
View changes

Copy link
Member

benjamingr left a comment

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

addaleax approved these changes Feb 14, 2018
View changes

@mcollina mcollina force-pushed the mcollina:end-this branch from 69ac34e to cc5937c Feb 15, 2018

@mcollina
@lpinca

lpinca approved these changes Feb 15, 2018
View changes

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`.

This comment has been minimized.

Sign in to view
Copy link
@lpinca

lpinca Feb 15, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Feb 16, 2018

Author Member

Done.

@BridgeAR BridgeAR added the author ready label Feb 16, 2018

@mcollina mcollina force-pushed the mcollina:end-this branch from cc5937c to 54facaa Feb 16, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Feb 16, 2018

@mscdex are you ok with this change then?
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented Feb 17, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/13240/
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Feb 19, 2018

Landed as f6721c2, 8118da7 and 3d93f39.

@mcollina mcollina closed this Feb 19, 2018

@mcollina mcollina deleted the mcollina:end-this branch Feb 19, 2018

mcollina added a commit that referenced this pull request Feb 19, 2018

@mcollina
stream: writable.end should return this. 
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>
f6721c2

mcollina added a commit that referenced this pull request Feb 19, 2018

@mcollina
http: OutgoingMessage.end() should return this 
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>
8118da7

mcollina added a commit that referenced this pull request Feb 19, 2018

@mcollina
http2: make response.end() return this 
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>
3d93f39
@benjamingr

This comment has been minimized.

Sign in to view
Copy link
Member

benjamingr commented Feb 19, 2018

Awesome work, adding "don't land on" tags.

@benjamingr benjamingr added dont-land-on-v4.x dont-land-on-v6.x dont-land-on-v8.x labels Feb 19, 2018

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

@mcollina @MayaLekova
stream: writable.end should return this. 
PR-URL: nodejs#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>
35ee7c7

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

@mcollina @MayaLekova
http: OutgoingMessage.end() should return this 
PR-URL: nodejs#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>
39e12f1

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

@mcollina @MayaLekova
http2: make response.end() return this 
PR-URL: nodejs#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>
1fb36fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.