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

this.res._headers has changed in node master #903

Closed
ilkkao opened this issue Feb 18, 2017 · 9 comments
Closed

this.res._headers has changed in node master #903

ilkkao opened this issue Feb 18, 2017 · 9 comments

Comments

@ilkkao
Copy link
Contributor

ilkkao commented Feb 18, 2017

Just compiled Node from the master branch and tried to use koa. It failed because the property values in this.res._headers are now arrays instead of strings.

$ node
> http = require('http');
> resp = new http.ServerResponse({});
> resp.setHeader('Content-Type', 'foo');
> result = resp._headers

In Node 7.x, result is { 'content-type': 'foo' }

In Node master (pre-8.0), result is { 'content-type': [ 'Content-Type', 'foo' ] }

The change was made here: nodejs/node@c00e4ad#diff-286202fdbdd74ede6f5f5334b6176b5cR406 (Node PR: nodejs/node#10558)

@mscdex noticed that it broke express and provided a fix https://github.com/jshttp/fresh/pull/20/files Something similar would be needed for koa.

I'll open this issue to make sure this issue is solved for koa in some way.

@mscdex
Copy link

mscdex commented Feb 18, 2017

I would probably wait until nodejs/node#10805 lands, which will provide more public API functions around accessing HTTP headers (there is a separate PR to deprecate _headers) for outgoing messages. Then you can do feature detection and use the new public API if it's available.

For some use cases though using functions such as the existing getHeader() is already enough.

@ilkkao
Copy link
Contributor Author

ilkkao commented Feb 20, 2017

I can prepare a PR when response.getHeaders from nodejs/node#10805 has landed

@mscdex
Copy link

mscdex commented Feb 21, 2017

Just a heads up, the PR has landed now in nodejs/node@3e6f103.

Additionally, nodejs/node#10941 will re-add _headers and _headerNames, but will result in runtime deprecation warnings.

@ilkkao
Copy link
Contributor Author

ilkkao commented Feb 21, 2017

Koa PR open: #907

@ilkkao
Copy link
Contributor Author

ilkkao commented Feb 26, 2017

Code in the master branch still uses this.res._headers directly. Shouldn't the fix go to master too?

@jonathanong
Copy link
Member

@ilkkao yes, that would be great!

@ilkkao
Copy link
Contributor Author

ilkkao commented Feb 26, 2017

I'll send a PR in few days

@JasonBoy
Copy link

JasonBoy commented Mar 2, 2017

@ilkkao getHeaders and getHeaderNames are added in node-v7.7, the comment Node < 8 in #907 could be updated to Node < 7.7?

@jonathanong
Copy link
Member

released in 2.1.0 and 1.3.0 thank you @ilkkao !!

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

Successfully merging a pull request may close this issue.

4 participants