fix: handle some edge cases with errors #247
Conversation
ronag
changed the title
ronag
force-pushed
the
fix-edge-cases
branch
2 times, most recently
from
715b4f8
to
458b2aa
Compare
Codecov Report
@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 75.31% 75.58% +0.26%
==========================================
Files 11 11
Lines 555 557 +2
Branches 126 126
==========================================
+ Hits 418 421 +3
- Misses 48 50 +2
+ Partials 89 86 -3
Continue to review full report at Codecov.
|
lib/ecstatic/status-handlers.js
Outdated
| @@ -2,58 +2,58 @@ | |||
|
|
|||
| const he = require('he'); | |||
|
|
|||
| function wrap(statusCode, fn) { | |||
| return (res, next, opts) => { | |||
| if (typeof next === 'function') { | |||
There was a problem hiding this comment.
This is inconsistent with the current behavior, right?
By my research, it looks like current behavior is to only check handleError in 404 cases and that it's not wired up literally anywhere else, and that prior to this change "next" was ignored, even though it was being passed in. 🤔 Not sure what the intent was there.
So there are a couple of things that would be good to see, if you're game:
- Making sure that the next() calls are consistent with handleError usage
- A documentation update to make this behavior more clear - shouldn't be a big deal, just describing that handleError: false will fall-through for whatever set of status codes
- Some tests that check that next is called properly depending on the state of handleError. I realize that the tests are kind of a shitshow. If this is a relatively low lift 🤞 great! If not, maybe not a big deal.
There was a problem hiding this comment.
Rather than making things complicated... I think I've fixed it so that it is consistent with the current behavior. Please see latest revision.
There was a problem hiding this comment.
I have to admit I don't understand the current behavior... but let's fix the current behavior first and then consider changing it...
There was a problem hiding this comment.
Honestly, I don't either. This is a project I wrote a long time ago and I haven't held the whole thing in my head for a while. Honestly there are a lot of things I would do different in a rewrite, but the test harness is awkward enough that I think a refactor that size would be challenging.
Anyway, ignoring next(), or removing its use from the call sites is fine by me, though one of us should flag this as an issue.
There was a problem hiding this comment.
Written up here: #251 feel free to add notes if you have any.
ronag
force-pushed
the
fix-edge-cases
branch
6 times, most recently
from
8d4e48e
to
34505d9
Compare
ronag
force-pushed
the
fix-edge-cases
branch
2 times, most recently
from
3477f55
to
351ee1d
Compare
|
Actually I think I figured it out pretty well with minimal changes. if Consistent behaviour only required a few changes which I believe to be rather appropriate. |
|
Merged manually and released as 4.1.1. Thanks! |
You can't write headers if headers are already sent... which is the case if e.g. the read stream fails (emit error) AFTER initial byte.
I'm not sure checking
writableis enough and also if headers are sent we still need to destroy the response.