feat(http/unstable): HttpError#7132
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7132 +/- ##
=======================================
Coverage 94.57% 94.57%
=======================================
Files 636 637 +1
Lines 52138 52153 +15
Branches 9399 9400 +1
=======================================
+ Hits 49311 49326 +15
Misses 2249 2249
Partials 578 578 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
What should happen here? import { HttpError } from '@std/http/unstable-error'
const { addr: { hostname, port } } = Deno.serve(() => new Response(null, {
status: 555,
}))
const res = await fetch(`http://${hostname}:${port}/`)
console.error(new HttpError(res.status))Current behaviour:
Also:
IMO current behaviour is fine as you can just cast |
|
On second thought, setting the |
fibibot
left a comment
There was a problem hiding this comment.
- JSDoc on the
initproperty (lines 119-121) claimsinit.statusandinit.statusTextalways reflect the standard HTTP values for the given status code. Neither is true after the recent commit:init = { status, ...options?.init }never assignsstatusText, andoptions.init.statuswins over the constructor'sstatus(spread comes last). Either re-add the auto-population, or rewrite the paragraph to describe the actual contract (init.statusdefaults tostatusand is overridable;init.statusTextis whatever the user passes).
- nit: while you're in there, the constructor doc block (lines 145-156) duplicates the class-level description verbatim. Constructor JSDoc can be a one-liner or just dropped — TypeDoc/LSP already shows the class doc on
new HttpError(...). - nit: subtests are named
"initialises with correct defaults"/"initialises with custom properties". The repo convention issymbolName() does X when Y— e.g."new HttpError() defaults message to STATUS_TEXT[status]".
bartlomieju
left a comment
There was a problem hiding this comment.
Quick re-review after the "Apply suggestions" commit (b3c043e). Three of fibibot's earlier points are only partially resolved — flagging them again with concrete fixes.
There's also a more substantive issue I want to surface that the previous round didn't catch: this.init = { status, ...options?.init } lets a user-supplied options.init.status silently override the constructor's status argument, leaving error.status !== error.init.status. Either the spread order is wrong (should be { ...options?.init, status }) or the type should disallow status inside options.init. Without a test pinning down the intent, this will go in as undefined behavior.
Apart from that the module is in good shape — small focused API, decent doc examples, browser-compatible, sits cleanly in the unstable-* namespace. Once the items below are addressed I'm happy to approve.
| * `init.status` always reflects the standard HTTP value for the given status | ||
| * code. |
There was a problem hiding this comment.
This claim is still misleading after the statusText removal:
init.statusis a number, so "reflects the standard HTTP value for the given status code" doesn't quite parse — that phrasing made sense forstatusText(where there's a canonical string like "Not Found"), but for.statusit's just "the same number".- More importantly, it's not always true: because
this.init = { status, ...options?.init }, a caller passing{ init: { status: 999 } }will seeerror.init.status === 999. See my comment on line 159 — until that's resolved, this sentence describes a contract that isn't enforced.
Suggested rewrite once the runtime contract is fixed:
init.statusalways equals thestatusargument passed to the constructor. OtherResponseInitfields (headers,statusText) come fromoptions.initif supplied.
| super(message, options); | ||
| this.name = this.constructor.name; | ||
| this.status = status; | ||
| this.init = { status, ...options?.init }; |
There was a problem hiding this comment.
Spread order looks reversed. With { status, ...options?.init }, a caller can do:
const err = new HttpError(500, undefined, { init: { status: 200 } });
// err.status === 500
// err.init.status === 200 // 😬That leaves the instance in an inconsistent state, and the JSDoc on line 119 documents the opposite invariant. I'd expect one of:
{ ...options?.init, status }—statusconstructor arg wins,initonly contributes otherResponseInitfields. This matches the doc.- Type-level fix — narrow
init?: Omit<ResponseInit, "status">inHttpErrorOptionsso this can't be expressed.
Whichever you pick, please add a test asserting error.status === error.init.status after passing init: { status: <different> }.
| /** | ||
| * Constructs a new instance. | ||
| * | ||
| * @param status The HTTP status code (e.g., 404, 500, 403) | ||
| * @param message Optional error message. Defaults to the standard status text for the given status code | ||
| * @param options Optional error options including cause and response init configuration | ||
| */ |
There was a problem hiding this comment.
fibibot's nit #2 was partially addressed (duplicated description gone) — but the three @param lines here just restate the class-level @param lines verbatim. The constructor JSDoc can be dropped entirely; TypeDoc/LSP will show the class doc on new HttpError(...). Keeping it adds drift risk (the @param lines are now redundant copies).
There was a problem hiding this comment.
The 3 @param lines are here because of the doc lint checker requires them in constructor JSDocs. Perhaps, this should be changed.
| import { assertEquals, assertInstanceOf } from "@std/assert"; | ||
| import { HttpError } from "./unstable_error.ts"; | ||
|
|
||
| Deno.test("HttpError initialises with correct defaults", () => { |
There was a problem hiding this comment.
fibibot's nit #3 wasn't addressed in the "Apply suggestions" commit — the test bodies were flattened out of the parent Deno.test("HttpError", t => ...) wrapper, but the names still read like step labels ("initialises with correct defaults") rather than the repo's symbolName() does X when Y convention. Suggested rewrites:
"HttpError initialises with correct defaults"→"new HttpError() defaults message to STATUS_TEXT[status]"(or split into"... sets status","... defaults message","... seeds init.status")"HttpError initialises with custom properties"→"new HttpError() forwards cause and init from options"
| (error.init.headers as Record<string, string>)["WWW-Authenticate"], | ||
| 'Basic realm="Secure Area"', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Missing coverage: an explicit test for the override semantics of options.init.status (see comment on unstable_error.ts:159). Whichever way that contract resolves, a test is the right artifact to lock it down so it doesn't silently regress later.
This PR brings back the
HttpErrorclass, which I regret removing in #3736 (sorry Kitson 🥲). It's a very versatile class that can be called anywhere in the call stack and handled in a unified way in the "master" request handler. I've been using this in my own projects and love it.This class is already implemented in Oak (similarly) and Fresh (a more minimal version).
I think it'd also be worth updating handlers within
@std/httpto throw errors instead of returning responses, allowing the dev to handle their own errors.Written by me. Checked by Claude Code.