-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
do not override already set content-type, fixes #1120 #1131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1131 +/- ##
=======================================
Coverage 99.73% 99.73%
=======================================
Files 5 5
Lines 372 372
=======================================
Hits 371 371
Misses 1 1
Continue to review full report at Codecov.
|
this might break apps if someone is relying on this behavior, so i would wait for semver major. |
unless we only set it to json if the type is not json-like |
I agree @fl0w |
Any news about this PR? @jonathanong |
Hi, it's been two years, any update on this PR? |
const res = response(); | ||
|
||
res.body = '<em>hey</em>'; | ||
assert.equal('text/html; charset=utf-8', res.header['content-type']); | ||
|
||
res.body = { foo: 'bar' }; | ||
assert.equal('application/json; charset=utf-8', res.header['content-type']); | ||
assert.equal('text/html; charset=utf-8', res.header['content-type']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test case is already updated, so this feature must already be live
semver major, #1114
This could be done as semver minor - if an application setting is addd, e.g.
app.responsOverride = true
or something.