fix(cbor): accept readonly input data in encoders#7148
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7148 +/- ##
=======================================
Coverage 94.61% 94.61%
=======================================
Files 634 634
Lines 51843 51850 +7
Branches 9346 9347 +1
=======================================
+ Hits 49050 49057 +7
Misses 2218 2218
Partials 575 575 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes denoland#5831 (for the `@std/cbor` module). `encodeCbor` / `encodeCborSequence` / `CborSequenceEncoderStream` / `CborArrayEncoderStream` never mutate their inputs, but the `CborType` parameter required mutable arrays, a mutable `Map`, and a mutable index signature. That meant `as const` literals, `ReadonlyMap` values, and `readonly T[]` arrays could not be passed without a cast, even though the runtime behaviour is identical. Add a new input-only type `CborInputType` that mirrors `CborType` but substitutes: - `CborType[]` -> `readonly CborInputType[]` - `Map<CborType, CborType>` -> `ReadonlyMap<CborInputType, CborInputType>` - `{ [k: string]: CborType }` -> `{ readonly [k: string]: CborInputType }` `CborType` (which is also the decoder output type) stays mutable, so this is fully backwards-compatible for existing callers and for the decode side. `CborStreamInput` is input-only and is widened in place. In the encoders, the existing `x instanceof Map` narrowing does not narrow `ReadonlyMap` on its own, so a tiny `isReadonlyMap` type predicate is added in `_common_encode.ts` and the relevant `#encodeArray` / `#encodeObject` stream helpers are widened to accept the readonly variants. `CborTag<T>`'s constraint is widened from `T extends CborType | CborStreamInput | CborStreamOutput` to also permit `CborInputType`. Mirrors the `@std/msgpack` change from denoland#5832. Regression tests added: - `encodeCbor()` accepts deeply-readonly `as const` literals. - `encodeCbor()` accepts readonly tuple literals. - `encodeCbor()` accepts `ReadonlyMap` input. - `encodeCbor()` accepts `{ readonly [k: string]: ... }` input. - `encodeCbor()` accepts a `CborTag` whose content is readonly. - `encodeCborSequence()` accepts a `readonly` array of values. All 89 CBOR module tests pass (24 pre-existing + 65 from related files; 6 new). `deno check cbor/` clean. `deno lint cbor/` clean. `deno fmt --check cbor/` clean.
8e60a59 to
80d0944
Compare
|
For the most part the pull request looks good. I'm personally not a fan of the name My only other concern is the comments in the tests you added. I feel like their content is pointless and that their meaning would be lost to time. |
…ments Per @BlackAsLight on denoland#7148: - Rename CborInputType -> ReadonlyCborType. Encoder signature now reads encodeCbor(value: CborType | ReadonlyCborType): Uint8Array (and the equivalent in encodeCborSequence), which keeps CborType prominent in the public API and clearly documents what additional shapes the encoder accepts. - Drop the descriptive comments on the new tests. The test names already describe what they cover, and the comments would lose context over time.
Closes #5831 for the
@std/cbormodule. Same shape as the@std/msgpackfix in #5832.The encoder functions and the encoder streams never mutate their inputs, but the existing
CborTyperequiresCborType[],Map<CborType, CborType>, and a mutable{ [k: string]: CborType }index signature. That means you can't pass anas constliteral, aReadonlyMap, or areadonly T[]toencodeCborwithout a cast, even though the runtime behavior is identical. Repro from the issue:I added a separate input-only type
CborInputTypeinstead of changingCborTypein place, becauseCborTypeis also the decoder output (decodeCborreturnsCborType) and changing it in place would be a breaking change for consumers that mutate decoded values.CborInputTypemirrorsCborTypebut withreadonly CborInputType[],ReadonlyMap<CborInputType, CborInputType>, and{ readonly [k: string]: CborInputType }. Encoders acceptCborInputType; decoders still returnCborType.CborStreamInputis input-only, so I widened it in place rather than adding a second type.CborTag<T>'s constraint widens fromT extends CborType | CborStreamInput | CborStreamOutputto also acceptCborInputTypesonew CborTag(1, readonlyValue)type-checks. This widens the constraint, doesn't narrow it, so existing instantiations stay valid.In
_common_encode.ts,x instanceof Mapdoesn't narrowReadonlyMapon its own (TS treats them as separate types), so there's a smallisReadonlyMappredicate. The#encodeArrayand#encodeObjectstream helpers widen to accept readonly variants.Existing callers passing mutable
CborTypekeep working because mutable subtypes assign to readonly supertypes.Regression tests in
encode_cbor_test.tscover deeply-readonlyas constliterals (exact repro from #5831), readonly tuples,ReadonlyMapinput, readonly index signatures, and aCborTagwith readonly content.encode_cbor_sequence_test.tscovers a readonly array of values.Ran locally:
deno check cbor/,deno lint cbor/,deno fmt --check cbor/all clean.deno test --allow-all cbor/shows 89 passed, 0 failed.