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.

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

http: add flushHeaders and deprecate flush #1156

Closed
wants to merge 1 commit into from
Closed

http: add flushHeaders and deprecate flush #1156

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

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.

req.setHeader('foo', 'bar');
req.flushHeaders();
});

Copy link
Member

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
Member

LGTM and seems like a reasonable change.

@iojs/collaborators Opinions?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 14, 2015

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

@Fishrock123
Copy link
Contributor

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

@dougwilson
Copy link
Member

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
Contributor

LGTM

@evanlucas
Copy link
Contributor

Wouldn't this be semver-major?

@dougwilson
Copy link
Member

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
Contributor

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

@tellnes
Copy link
Contributor

tellnes commented Mar 16, 2015

LGTM

bnoordhuis pushed a commit that referenced this pull request Mar 17, 2015
PR-URL: #1156
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Tellnes <christian@tellnes.no>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@bnoordhuis
Copy link
Member

Thanks Yosuke, landed in b2e00e3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants