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

Support upcoming changes in node #20

Closed
wants to merge 1 commit into from
Closed

Support upcoming changes in node #20

wants to merge 1 commit into from

Conversation

mscdex
Copy link

@mscdex mscdex commented Jan 14, 2017

No description provided.

@dougwilson
Copy link
Contributor

Thanks, but I don't understand why the change is necessary. You can see on the README the expected object for what a user should be passing in. They shouldn't be passing in anything where the value is an array.

@dougwilson dougwilson self-assigned this Jan 14, 2017
@dougwilson dougwilson self-requested a review January 14, 2017 00:21
@dougwilson dougwilson added the pr label Jan 14, 2017
@mscdex
Copy link
Author

mscdex commented Jan 14, 2017

The issue is that express is blindly passing res._headers as the second argument to fresh(). I can submit a PR to express that basically makes a clone of ._headers with string values before passing to fresh(), but I figured that would both be "expensive" and there may be others using fresh similarly and changing it one spot would be easier/better.

@dougwilson
Copy link
Contributor

I mean, that sounds like those module are where the change belongs, because this module has never documented ever being able to pass in that object (also not tested). If those modules upstream wanted to take a short cut that was working, that's fine, but if their shortcut stopped working, they should change it there.

Because even this, this PR would then be missing the documentation to document that the objects passed in could now take arrays, which once you document the API looks like that, it doesn't really even make sense any longer...

@mscdex
Copy link
Author

mscdex commented Jan 14, 2017

Alright, will open a PR against express then.

@mscdex mscdex closed this Jan 14, 2017
@dougwilson dougwilson removed their request for review January 14, 2017 00:32
@dougwilson
Copy link
Contributor

Gotcha. I look at express with blame and it looks like express has done that basically forever, lol. I know I've seen usages of res._headers in other places, especially in code created before res.getHeader existed, so I wouldn't be surprised if other things are affected similarly. Where can I see the changes made to the storage of res._headers?

I know times where I have personally reached for res._headers was a simple way to enumerate all the headers in a response, and I don't see any public replacement for that. Do you know how much core (at least on npm) has been reaching for _headers to at least have some kind of replacement public API? I know it's a private API and shame on code that uses it, of course, but sometimes there isn't another immediate option :/

Also, what version of Node.js is this change in (or is it just pending in a PR somewhere)?

@mscdex
Copy link
Author

mscdex commented Jan 14, 2017

Node PR is at: nodejs/node#10558

You can still iterate over it (although it is private) like before, but the thing that changed is the value. Basically instead of storing the original cased header names in a separate object (._headerNames), they're now stored alongside the value in ._headers in an array. For example: { etag: ['ETag', '12345'] }. A public API that returns a stable view of the request/response headers is the way forward (like how we do with eventEmitter.listeners()). If you want to submit a PR to add such functionality, go for it. Otherwise I may do so whenever I get a chance next.

Currently the change is only in master and I've labeled it as semver-major now. So it would be in v8.0.0 at the earliest.

@mscdex
Copy link
Author

mscdex commented Jan 14, 2017

FWIW I went ahead and submitted a PR to add a .getHeaders() method to allow users to avoid having to resort to accessing a private/underscored property: nodejs/node#10805

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.

2 participants