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

http: don't confuse automatic headers for others #828

Closed
wants to merge 1 commit into from

Conversation

tellnes
Copy link
Contributor

@tellnes tellnes commented Feb 13, 2015

If you set a custom http header which includes eg. the string Date, then http will not automatic send the Date header. This is also true for other automatic http headers.

Btw, there is no header named Proxy-Connections which is used in the test I'm changing.

If you set a custom http header which includes eg. the string `Date`,
then http will not automatic send the `Date` header.

This is also true for other automatic http headers.
const connectionExpression = /Connection/i;
const transferEncodingExpression = /Transfer-Encoding/i;
const connectionExpression = /^Connection$/i;
const transferEncodingExpression = /^Transfer-Encoding$/i;
const closeExpression = /close/i;
Copy link
Member

Choose a reason for hiding this comment

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

why is this left out?

Copy link
Member

Choose a reason for hiding this comment

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

oh, nmind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a possible bug also. Maybe this regexp:

/(^|,(\ )*)close($|,)/i

@brendanashworth
Copy link
Contributor

LGTM besides my comment. 👍

@Fishrock123
Copy link
Contributor

LGTM, but it would be nice to have one for close also.

@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Feb 13, 2015
@brendanashworth
Copy link
Contributor

I believe this could be interpreted as a non-backwards compatible change, if a developer were to rely on not sending a Date header if he set an X-Date header himself. Opinions on that?

@chrisdickinson
Copy link
Contributor

@brendanashworth From my perspective, that behavior is a bug.

@brendanashworth
Copy link
Contributor

@chrisdickinson it certainly isn't documented anywhere and I pity the developer that would call it a feature, but there is always the possibility.

@tellnes
Copy link
Contributor Author

tellnes commented Feb 14, 2015

@brandon-beacher A lot of bugs can be called a feature by somebody else.

@Fishrock123 I think a solution for close belongs in another fix with.

@brandon-beacher
Copy link
Contributor

@tellnes I think you meant to mention someone else?

@tellnes
Copy link
Contributor Author

tellnes commented Feb 14, 2015

@brandon-beacher Yes I did. Sorry about that.

@brendanashworth The mention was for you.

@brendanashworth
Copy link
Contributor

@tellnes I'm aware of that, but it at least deserved to be brought up. Any change that could break semver should be looked at as if it would break semver.

It does change end functionality of this library though. Wouldn't break it, but thats at least one example.

I'm still +1 on the change though, don't get me wrong.

@tellnes
Copy link
Contributor Author

tellnes commented Feb 16, 2015

So the question is then. I would say this is a bug fix, and therefore should be tagged as a patch. It is an internal change that fixes incorrect behavior.

But since it might break code, maybe it should be semver minor? Personally I don't think anyone is depending on this behavior deliberate, but someone does probably it in unit tests like the one I've changed.

@brendanashworth
Copy link
Contributor

To be honest, the I could only find one library that would change functionality, and it wouldn't break it. I'm up for a patch and could merge this in a few days unless anyone objects.

tellnes added a commit that referenced this pull request Feb 28, 2015
If you set a custom http header which includes eg. the string `Date`,
then http will not automatically send the `Date` header.

This is also true for other automatic http headers.

PR-URL: #828
Reviewed-By: Brendan Ashworth <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@brendanashworth
Copy link
Contributor

Thanks @tellnes, merged as semver-patch in 675cffb . Just in time for v1.4.2, ping #995.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants