feat: ignore set/remove header/status when header sent

[dead-horse]

from #1078

we should not throw headers sent error to make ctx.responed and ctx.flushHeaders more safety.

[dead-horse]

we shouldn't return here because there are some specific logic for streams below.

[codecov]

Codecov Report

Merging #1137 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1137      +/-   ##
==========================================
+ Coverage   99.73%   99.73%   +<.01%     
==========================================
  Files           5        5              
  Lines         372      374       +2     
==========================================
+ Hits          371      373       +2     
  Misses          1        1

Impacted Files	Coverage Δ
lib/response.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0923ef6...e3f56a2. Read the comment docs.

[fengmk2]

LGTM

[fengmk2]

Should patch this feature into koa v1

[dead-horse]

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 ctx.flushHeader or ctx.responed.

[fl0w]

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/) {
    // ...
  }
}

[dead-horse]

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.

[dead-horse]

Any suggestions? Or I'll merge as a minor change and port it to 1.x.

[noinkling]

This error can be a very useful indicator that there's a flaw in my app's logic somewhere, therefore I'm a bit wary of this change because now it will be harder to detect those sorts of bugs. Is there any way we could have the best of both worlds?

[dead-horse]

@noinkling if you don't use ctx.responed or ctx.flushHeader, there is nothing to worry about.

if you do use these methods, you should take care of them and avoid to get into this situation.

[noinkling]

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.