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

Don't access res._headers directly when helpers exist #907

Closed
wants to merge 2 commits into from

Conversation

ilkkao
Copy link
Contributor

@ilkkao ilkkao commented Feb 21, 2017

Node HTTP module response._headers is considered internal to node itself. Its value will
change in a backwards incompatible way in the future node releases.

Use the documented accessor functions instead when they are available.

lib/context.js Outdated
@@ -116,7 +116,12 @@ var proto = module.exports = {
}

// unset all headers, and set those specified
this.res._headers = {};
if (this.res.getHeaderNames) {
this.res.getHeaderNames().forEach(function(name) { this.removeHeader(name) }, this.res);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line gets tested when the tests a are run with node 8 (test/context/onerror.js). I did a manual test run using node master branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no removeAllHeaders(), hence the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then again coverage was 100%, it can't drop. Added a test.

@codecov
Copy link

codecov bot commented Feb 21, 2017

Codecov Report

Merging #907 into master will decrease coverage by -0.24%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #907      +/-   ##
==========================================
- Coverage     100%   99.76%   -0.24%     
==========================================
  Files           4        4              
  Lines         420      424       +4     
  Branches      102      104       +2     
==========================================
+ Hits          420      423       +3     
- Misses          0        1       +1
Impacted Files Coverage Δ
lib/response.js 100% <100%> (ø)
lib/context.js 96.87% <66.66%> (-3.13%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3029f5...368038d. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 21, 2017

Codecov Report

Merging #907 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #907   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         420    424    +4     
  Branches      102    104    +2     
=====================================
+ Hits          420    424    +4
Impacted Files Coverage Δ
lib/response.js 100% <100%> (ø)
lib/context.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3029f5...1e5a861. Read the comment docs.

res._headers is considered internal to node itself. Its value will
change in a backwards incompatible way in the future node releases.

Use the documented helper functions instead when they are available.
@ilkkao ilkkao force-pushed the ilkkao/fix-headers branch from 368038d to c2550c2 Compare February 21, 2017 07:53
@ilkkao ilkkao force-pushed the ilkkao/fix-headers branch from c2550c2 to 1e5a861 Compare February 21, 2017 07:54
@jonathanong jonathanong changed the base branch from master to v1.x February 25, 2017 06:26
@jonathanong
Copy link
Member

I want to merge this, but I want to test against node nightly. anyone know if there's an easy way to test against node nightly on travis?

@dougwilson
Copy link
Contributor

@jonathanong
Copy link
Member

@dougwilson you are the best

@jonathanong
Copy link
Member

#913 91c403a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants