fix($http): properly access request headers with mixed case #10883
Conversation
| @@ -71,14 +71,16 @@ function headersGetter(headers) { | |||
| var headersObj = isObject(headers) ? headers : undefined; | |||
|
|
|||
| return function(name) { | |||
| var existingHeaderName, lowercaseName = lowercase(name); | |||
There was a problem hiding this comment.
Can you please move this directly above the for .. in? Or even inside, browsers should be able to optimize this.
There was a problem hiding this comment.
Hmm, I would rather not as if / for don't create lexical scope in JS. I know that browsers will do it for you but I think that most of the code in AngularJS is declaring variables at the lexical scope, no?
Anyway, I don't feel strong about it, let's just be consistent.
There was a problem hiding this comment.
I hadn't thought about that actually. I just find it less unreadable - as the variable is only used in the loop.
Anyway, it's your call. However, if you don't change it, can you give the variable its own var? I think that most new code follows this pattern, and I think at least @caitp also prefers it.
There was a problem hiding this comment.
@Narretz please have a look at the existing in methods in $http - as far as I can see there is not consistent style for declaring vars but many methods declare multiple vars using one var keyword (example). One again, I'm all for consistency and clear code standards, but given lack of consistency in the current code base, I don't think style discussion should be happening to in this PR.
My point is: I'm all for common coding standards and consistent coding patterns and we can raise it on the team meeting. But given that this PR doesn't introduce different coding style, as compared to the existing code in this file, I don't think that this PR is the best place to discuss personal coding preferences... At the end of the day I don't care either way as long we all on the same page.
There was a problem hiding this comment.
Fair enough. Let's just leave it as is.
|
@petebacondarwin would you mind taking a quick look at this one? |
|
Style discussions aside - Wouldn't it be more performant to simply lower case the header names when parsing them in the first place? |
|
@petebacondarwin here we are talking about request headers so the ones provided by a user. In this sense we are not parsing anything. |
pkozlowski-opensource
force-pushed
the
issue10881
branch
from
b7b3300 to
8b6f8a0
Compare
|
@petebacondarwin I did the changes we've discussed during the hangout review. Mind taking another look? |
|
LGTM |
Fixes #10881