-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Use headersSent instead of _header #129
Use headersSent instead of _header #129
Conversation
Hm, I just noticed that I branched from my other changes. Should I remove those changes from this PR? |
Hi @maritz I can try to cut them out myself during the merge, but if you can remove them, that may be good especially in case something else changes (either in the other PR or between that PR merging and the major release to include this change). |
d7bb81b
to
cd957aa
Compare
de4f632
to
24a6740
Compare
@dougwilson I rebased and cut out the extra commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this change be better?
It will be backwards compatible with Node.js v0.8 and not incur a semver major.
For background information regarding We can also add test cases to assert the equivalence (if really required). |
…/use_headersSent_instead_of__header
@lamweili I don't think we need a test for this, this change was also made in pillarjs/finalhandler#15. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
All good, was coming from test/code coverage perspective. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(can't approve cause I don't have explicit access to this repository)
😮💨
thanks @maritz |
As discussed in #127 this PR now includes the changes that do break node v0.8 and has tests.
I did not remove the travis config for v0.8 since I'm not sure that's something that should be in a PR.