fix(http): getCookies returns Partial<Record<string, string>> (closes #7053)#7131
fix(http): getCookies returns Partial<Record<string, string>> (closes #7053)#7131MukundaKatta wants to merge 1 commit into
Conversation
…enoland#7053) The previous return type `Record<string, string>` was a lie: arbitrary key access can yield `undefined` because the function only populates keys present in the Cookie header. Consumers without `noUncheckedIndexedAccess` enabled would unsafely treat missing-cookie reads as guaranteed strings. Switch to `Partial<Record<string, string>>`. The runtime behavior is unchanged (the returned object is still null-prototyped, so missing-key access really does yield `undefined` rather than an inherited Object prototype method), but TypeScript now reflects that. - update return type + JSDoc to spell out the contract - regression test asserts the new exact type via IsExact<> - keeps the existing null-prototype check so behaviour stays pinned
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7131 +/- ##
=======================================
Coverage 94.61% 94.61%
=======================================
Files 634 634
Lines 51801 51802 +1
Branches 9329 9329
=======================================
+ Hits 49011 49012 +1
Misses 2216 2216
Partials 574 574 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lunadogbot
left a comment
There was a problem hiding this comment.
Type matches the runtime: the object is null-prototyped, so missing-key access really is undefined. JSDoc and the IsExact<> test pin the new shape, and the unstable_signed_cookie.ts examples already guard with if (signedCookie === undefined), so they keep type-checking.
One thing for your call: @std/http is v1.1.0 and getCookies is in mod.ts. Even with runtime unchanged, const s: string = getCookies(h).foo stops compiling on this PR — that's a type-level break for callers without noUncheckedIndexedAccess. Worth deciding whether the title should be BREAKING(http): rather than fix(http):.
- nit: the new prose block in JSDoc (lines 236-241) duplicates what
@returnnow says about null-prototype +string | undefinedsemantics. One could go.
lunadogbot
left a comment
There was a problem hiding this comment.
CI is green now, promoting prior review to APPROVE.
|
@bartlomieju this is ready to merge |
Closes #7053 (and the runtime/TS gap originally tracked in #6852).
What's wrong
The previous return type
Record<string, string>told TypeScript that any key access yielded astring, but the function only populates keys present in theCookieheader. Consumers withoutnoUncheckedIndexedAccessenabled would unsafely treat missing-cookie reads as guaranteed strings:Fix
Return type is now
Partial<Record<string, string>>. Runtime behaviour is unchanged (the returned object is still null-prototyped, so missing-key access really does yieldundefinedrather than an inheritedObject.prototypemethod), but TypeScript now reflects that.What changed
getCookiesreturn type:Record<string, string>→Partial<Record<string, string>>string | undefinedaccess semantics)IsExact<>so a future revert can't sneak pastTest plan
deno test http/cookie_test.ts— 10/10 passing locally with full type-checkWhy now
#7053 explicitly notes the previous attempt left this "still" wrong — the type and the runtime were out of sync.