-
-
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
feat: ignore set/remove header/status when header sent #1137
Conversation
@@ -133,8 +134,6 @@ module.exports = { | |||
const original = this._body; | |||
this._body = val; | |||
|
|||
if (this.res.headersSent) return; |
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.
we shouldn't return here because there are some specific logic for streams below.
Codecov Report
@@ Coverage Diff @@
## master #1137 +/- ##
==========================================
+ Coverage 99.73% 99.73% +<.01%
==========================================
Files 5 5
Lines 372 374 +2
==========================================
+ Hits 371 373 +2
Misses 1 1
Continue to review full report at Codecov.
|
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.
LGTM
Should patch this feature into koa v1 |
I'd like to consider it as a minor version change because it only avoid throw exception when set header after header sent, which bothered developers using 1.x and 2.x. And it is harmless if you don't break koa's response handling by |
assert('number' == typeof code, 'status code must be a number'); | ||
assert(statuses[code], `invalid status code: ${code}`); | ||
assert(!this.res.headersSent, 'headers have already been sent'); |
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.
I understand my opinion wasn't asked for, but I'm considering if this could be a breaking change.
Is there any reason a user would rely on something like:
try {
ctx.status = 201
} catch (err) {
if (err.message.match(/headers have already been sent/) {
// ...
}
}
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.
yes, it is hard to make this decision, but even in this situation, you can do nothing but log this error because the http header has already sent.
Any suggestions? Or I'll merge as a minor change and port it to 1.x. |
|
@noinkling if you don't use if you do use these methods, you should take care of them and avoid to get into this situation. |
In my case it's libraries that are doing it (e.g. wrapped Express middlewares), where I shouldn't be trying to mess with the response later in the chain. But I see your point that there should be no difference under normal usage, so I withdraw my comment. |
from #1078
we should not throw headers sent error to make
ctx.responed
andctx.flushHeaders
more safety.