Skip to content

http: add flushHeaders and deprecate flush#1156

Closed
yosuke-furukawa wants to merge 1 commit intonodejs:v1.xfrom
yosuke-furukawa:fix/add_alias_http_flush_headers
Closed

http: add flushHeaders and deprecate flush#1156
yosuke-furukawa wants to merge 1 commit intonodejs:v1.xfrom
yosuke-furukawa:fix/add_alias_http_flush_headers

Conversation

@yosuke-furukawa
Copy link
Copy Markdown
Member

@yosuke-furukawa yosuke-furukawa commented Mar 14, 2015

I found a different API from Node.js v0.12 and io.js.
the detail is here nodejs/node-v0.x-archive@89f3c90

http module in io.js has a method req.flush.
Node.js v0.12 also has a same behavior method but the different name req.flushHeaders.

the difference may cause to block migrations from Node.js v0.12 to io.js.
But if we rename req.flush to req.flushHeaders now, we should update the major version.

So I add req.flushHeaders method and deprecate req.flush.

If we bump the io.js master version, we would be better to rethink req.flush is removed.

bnoordhuis
bnoordhuis reviewed Mar 14, 2015
View reviewed changes
Comment thread test/parallel/test-http-flush-headers.js Outdated
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Mar 14, 2015

Choose a reason for hiding this comment

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

Tiny nit: blank line at EOF.

@bnoordhuis bnoordhuis added semver-minor PRs that contain new features and should be released in the next minor version. http Issues or PRs related to the http subsystem. labels Mar 14, 2015
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Mar 14, 2015

LGTM and seems like a reasonable change.

@iojs/collaborators Opinions?

@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Mar 14, 2015

LGTM. This actually seems more comprehensive than the original commit in joyent/node.

@Fishrock123
Copy link
Copy Markdown
Contributor

Fishrock123 commented Mar 14, 2015

@dougwilson I know I've seen flush around express before, how annoying is a deprecation like this?

@dougwilson
Copy link
Copy Markdown
Member

dougwilson commented Mar 14, 2015

This change is probably better for Express, because we don't use this method directly (yet), but also our compression middleware actually adds a res.flush() method for users to flush the zlib buffer to the client. With this change, it means io.js users of compression can more easily access this feature, since we won't be stomping on the method any longer :)

@Fishrock123
Copy link
Copy Markdown
Contributor

Fishrock123 commented Mar 14, 2015

LGTM

@evanlucas
Copy link
Copy Markdown
Contributor

evanlucas commented Mar 14, 2015

Wouldn't this be semver-major?

@dougwilson
Copy link
Copy Markdown
Member

dougwilson commented Mar 14, 2015

Deprecations are only semver minor. See rule #7 on http://semver.org/

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated.

Just making something as deprecated is not backwards-incompatible, as all code using a deprecation function works exactly the same.

@evanlucas
Copy link
Copy Markdown
Contributor

evanlucas commented Mar 14, 2015

Ah, didn't see that flush() was still there but deprecated :/

@tellnes
Copy link
Copy Markdown
Contributor

tellnes commented Mar 16, 2015

LGTM

@brendanashworth brendanashworth mentioned this pull request Mar 16, 2015
Closed
bnoordhuis pushed a commit that referenced this pull request Mar 17, 2015
@yosuke-furukawa @bnoordhuis
http: add flushHeaders and deprecate flush
b2e00e3 
PR-URL: #1156
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Christian Tellnes <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Mar 17, 2015

Thanks Yosuke, landed in b2e00e3.

@bnoordhuis bnoordhuis closed this Mar 17, 2015
@julianduque julianduque mentioned this pull request Mar 20, 2015
Closed
@jarrodconnolly jarrodconnolly mentioned this pull request Aug 1, 2015
Closed
fijimunkii added a commit to fijimunkii/sse-channel that referenced this pull request May 11, 2017
@fijimunkii
Use flushHeaders method
d743ec9 
As `flush` was deprecated

https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L892
nodejs/node#1156
@fijimunkii fijimunkii mentioned this pull request May 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yosuke-furukawa @bnoordhuis @cjihrig @Fishrock123 @dougwilson @evanlucas @tellnes