http: don't confuse automatic headers for others #828
Conversation
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; |
There was a problem hiding this comment.
That is a possible bug also. Maybe this regexp:
/(^|,(\ )*)close($|,)/i
|
LGTM besides my comment. 👍 |
|
LGTM, but it would be nice to have one for |
|
I believe this could be interpreted as a non-backwards compatible change, if a developer were to rely on not sending a |
|
@brendanashworth From my perspective, that behavior is a bug. |
|
@chrisdickinson it certainly isn't documented anywhere and I pity the developer that would call it a feature, but there is always the possibility. |
|
@brandon-beacher A lot of bugs can be called a feature by somebody else. @Fishrock123 I think a solution for |
|
@tellnes I think you meant to mention someone else? |
|
@brandon-beacher Yes I did. Sorry about that. @brendanashworth The mention was for you. |
|
@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. |
|
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. |
|
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. |
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]>
If you set a custom http header which includes eg. the string
Date, then http will not automatic send theDateheader. This is also true for other automatic http headers.Btw, there is no header named
Proxy-Connectionswhich is used in the test I'm changing.