fix: add TypeScript overload signatures for accepts methods#494
fix: add TypeScript overload signatures for accepts methods#494claygeo wants to merge 3 commits into
Conversation
The Request interface defines accepts, acceptsEncodings, acceptsCharsets, and acceptsLanguages with a single signature that returns the broad union string | boolean | string[]. The underlying implementation already supports multiple call patterns with different return types: - No args: returns all accepted values as string[] - With args: returns best match as string, or false This adds overload signatures so TypeScript narrows the return type based on how the method is called, matching Express behavior. Also narrows AcceptsReturns from boolean to the literal false type since these methods never return true. Closes tinyhttp#396
…asts After the four accepts methods switched to inline overload signatures, the AcceptsReturns alias is no longer referenced anywhere; remove the dead code. Also wrap the acceptsCharsets/Encodings/Languages cast lines to satisfy biome's 120-char lineWidth.
claygeo
force-pushed
the
fix/accepts-type-overloads
branch
from
fcee88b to
30f3021
Compare
|
Friendly bump on this one. Rebased onto the latest Still closes #396 (open). The change is type-only with no runtime change. The existing test suite passes and the overloads narrow correctly: no-arg returns @v1rtl when you have a moment, would you be open to a re-review? Happy to adjust anything. |
| req.acceptsLanguages = (...languages) => getAcceptsInstance().languages(languages) | ||
| req.accepts = ((...types: string[]) => getAcceptsInstance().types(types)) as Request['accepts'] | ||
| req.acceptsCharsets = ((...charsets: string[]) => | ||
| getAcceptsInstance().charsets(charsets)) as Request['acceptsCharsets'] |
There was a problem hiding this comment.
Does it have to be invoked on every call, or it could be just statically defined and then reused for all Accepts methods?
There was a problem hiding this comment.
Good question. Short version: it's already defined once and shared across all four methods, just lazily. Three things going on:
-
It's already reused across all four methods.
getAcceptsInstance()memoizes a singleAcceptsinstance in the closure (if (!acceptsInstance) acceptsInstance = new Accepts(req)), soaccepts/acceptsCharsets/acceptsEncodings/acceptsLanguagesall share one instance per request. It's never recreated per method or per call. -
It can't be hoisted to module scope.
Acceptsis request-bound: it capturesreq.headersand builds aNegotiatorover the request. So each request needs its own instance; a module-level singleton would leak one request's headers into the next. -
Why it's behind the getter (lazy) rather than an eager
const. That's the hot-path optimization from perf: optimize hot path performance in request handling #485.extendMiddlewareruns on every request, but most requests never call any accepts method, so the lazy getter skips thenew Accepts(req)+new Negotiator(req)allocations entirely for them. When a method is called, the getter is just a closure call plus a null check, and the instance is built at most once. (BothAcceptsandNegotiatorconstructors only store references; no header parsing happens until a method actually runs.)
Worth noting this PR is type-only. The getter predates it (added in #485); I only retyped the four assignment lines so the new overloads narrow correctly (no-arg returns string[], with-arg returns string | false). Runtime is unchanged.
If you'd rather drop the getter for readability and use an eager const acceptsInstance = new Accepts(req), I'm happy to do that here. The only tradeoff is it adds those two small allocations back to every request even when content negotiation isn't used, which is why I left #485's lazy form in place. Your call either way.
Verified locally on the branch: tsc is clean and the accepts/request/req suites pass (115 tests).
v1rtl
left a comment
v1rtl
left a comment
There was a problem hiding this comment.
Overall good, one tiny question
2 participants
Problem
The
Requestinterface definesaccepts,acceptsEncodings,acceptsCharsets, andacceptsLanguageswith a single signature:This means TypeScript can't narrow the return type based on usage. Users must cast or type-guard every call:
The underlying
Acceptsclass already handles all call patterns correctly and returns different types based on input.Solution
Add TypeScript overload signatures that narrow the return type:
Also narrows
AcceptsReturnsfrombooleanto the literalfalsetype, since these methods returnfalse(nottrue) when no match is found. This matches Express's type definitions.No runtime changes. The implementation already supports all call patterns. This is purely a type-narrowing improvement.
Changes
packages/app/src/request.ts— Added overload signatures for all four accepts methods, narrowedAcceptsReturnsto usefalseinstead ofbooleanCloses #396