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

Ignore the case when getting a response header #2250

Conversation

emelois
Copy link
Contributor

@emelois emelois commented Jun 6, 2016

According to the RFC: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
"Field names are case-insensitive".
For instance, Google Cloud Endpoints use "content-type" in the response.

@ns-bot
Copy link

ns-bot commented Jun 6, 2016

Can one of the admins verify this patch?

4 similar comments
@ns-bot
Copy link

ns-bot commented Jun 6, 2016

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Jun 6, 2016

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Jun 6, 2016

Can one of the admins verify this patch?

@ns-bot
Copy link

ns-bot commented Jun 6, 2016

Can one of the admins verify this patch?

@ns-bot ns-bot added the cla: yes label Jun 6, 2016
&& !this._errorFlag
) {
return this._headers[header];
for (var i in this._headers) {
if (i.toLowerCase() === header.toLowerCase()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could cache the call to header.toLowerCase() instead of make it always.

@hshristov
Copy link
Contributor

Is it possible to add a unit-test?

According to the RFC: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
"Field names are case-insensitive".
For instance, Google Cloud Endpoints use "content-type" in the response.
@emelois emelois force-pushed the emelois/response-headers-case-insensitive branch from 37e6e3d to c997427 Compare June 17, 2016 07:57
@emelois
Copy link
Contributor Author

emelois commented Jun 17, 2016

@hshristov I cached the lowercase header and added a unit test.

@vchimev
Copy link
Contributor

vchimev commented Jun 20, 2016

run ci

@hshristov hshristov merged commit 6282bae into NativeScript:master Jun 20, 2016
@hshristov
Copy link
Contributor

@emelois Thank you for your contribution. PR will be live with the next official release - 2.1.0

@lock
Copy link

lock bot commented Aug 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Aug 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants