From 0fb245342973e75178606898074162da24314a55 Mon Sep 17 00:00:00 2001 From: arturovt Date: Fri, 14 Aug 2026 00:47:55 +0300 Subject: [PATCH] fix(common): preserve literal key union in KeyValuePipe.transform() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when you passed an object typed like Record<'a' | 'b', number> into the `keyvalue` pipe, TypeScript would "forget" that the keys could only ever be 'a' or 'b', and just tell you the key was a plain `string` instead. So code like this used to fail to compile, even though it's correct: ```ts const input: Record<'a' | 'b', number> = {a: 1, b: 2}; const result = pipe.transform(input); const key: 'a' | 'b' = result[0].key; // error: string is not 'a' | 'b' ``` This happened because the pipe has multiple overloaded versions of transform(), and TypeScript checks them top to bottom, using the first one that matches. The "number keys" overload was listed first, and it happened to also match string-keyed objects by accident, so it "won" before the correct "string keys" overload ever got a chance to run. The fix just reorders those two overloads so the string-keys one is checked first. Nothing about runtime behavior changes — objects with actual numeric keys (e.g. Record<1 | 2, string>) still correctly report their keys as plain `string`, matching what Object.keys() really returns at runtime. --- goldens/public-api/common/index.api.md | 8 ++--- packages/common/src/pipes/keyvalue_pipe.ts | 27 ++++++++++------ .../common/test/pipes/keyvalue_pipe_spec.ts | 32 +++++++++++++++++++ 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/goldens/public-api/common/index.api.md b/goldens/public-api/common/index.api.md index 211fcfa68334..4a5a43b7a270 100644 --- a/goldens/public-api/common/index.api.md +++ b/goldens/public-api/common/index.api.md @@ -379,18 +379,18 @@ export class KeyValuePipe implements PipeTransform { // (undocumented) transform(input: ReadonlyMap, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array>; // (undocumented) - transform(input: Record, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array>; - // (undocumented) transform(input: Record | ReadonlyMap, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array>; // (undocumented) + transform(input: Record, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array>; + // (undocumented) transform(input: null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): null; // (undocumented) transform(input: ReadonlyMap | null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array> | null; // (undocumented) - transform(input: Record | null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array> | null; - // (undocumented) transform(input: Record | ReadonlyMap | null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array> | null; // (undocumented) + transform(input: Record | null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null): Array> | null; + // (undocumented) transform(input: T, compareFn?: T extends object ? (a: T[keyof T], b: T[keyof T]) => number : never): T extends object ? Array> : null; // (undocumented) static ɵfac: i0.ɵɵFactoryDeclaration; diff --git a/packages/common/src/pipes/keyvalue_pipe.ts b/packages/common/src/pipes/keyvalue_pipe.ts index 17e61214d401..8278f432075f 100644 --- a/packages/common/src/pipes/keyvalue_pipe.ts +++ b/packages/common/src/pipes/keyvalue_pipe.ts @@ -75,14 +75,23 @@ export class KeyValuePipe implements PipeTransform { input: ReadonlyMap, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, ): Array>; - transform( - input: Record, - compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, - ): Array>; + // TypeScript tries overloads top-to-bottom and stops at the first match. A + // plain object with string-literal keys (e.g. `Record<'a' | 'b', V>`) also + // happens to satisfy the more general `K extends number` overload below once + // inference falls back to its constraint — so if that overload is checked + // first, it wins by accident and silently widens `key` from `'a' | 'b'` to + // plain `string`. Putting the string overload first means it claims + // string-keyed input before the number overload ever gets a chance, while + // genuinely numeric-keyed input (which fails `K extends string`) still falls + // through to the number overload exactly as before. transform( input: Record | ReadonlyMap, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, ): Array>; + transform( + input: Record, + compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, + ): Array>; transform( input: null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, @@ -91,16 +100,16 @@ export class KeyValuePipe implements PipeTransform { input: ReadonlyMap | null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, ): Array> | null; - transform( - input: Record | null | undefined, - compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, - ): Array> | null; - transform( input: Record | ReadonlyMap | null | undefined, compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, ): Array> | null; + transform( + input: Record | null | undefined, + compareFn?: ((a: KeyValue, b: KeyValue) => number) | null, + ): Array> | null; + transform( input: T, compareFn?: T extends object ? (a: T[keyof T], b: T[keyof T]) => number : never, diff --git a/packages/common/test/pipes/keyvalue_pipe_spec.ts b/packages/common/test/pipes/keyvalue_pipe_spec.ts index 9b4727f5eef5..d63e39a82921 100644 --- a/packages/common/test/pipes/keyvalue_pipe_spec.ts +++ b/packages/common/test/pipes/keyvalue_pipe_spec.ts @@ -100,6 +100,38 @@ describe('KeyValuePipe', () => { expect(pipe.transform(value)).toEqual(null); }); + it('should preserve a literal key union instead of widening it to string', () => { + const pipe = new KeyValuePipe(defaultKeyValueDiffers); + const input: Record<'a' | 'b', number> = {a: 1, b: 2}; + const result = pipe.transform(input); + + // Compile-time check: if `key` had widened back to plain `string`, this + // assignment would fail to compile — that's the actual bug from #43883. + const key: 'a' | 'b' = result[0].key; + + expect(key).toBe('a'); + expect(result).toEqual([ + {key: 'a', value: 1}, + {key: 'b', value: 2}, + ]); + }); + + it('should still collapse a numerically-keyed object to string keys', () => { + const pipe = new KeyValuePipe(defaultKeyValueDiffers); + const input: Record<1 | 2, string> = {1: 'one', 2: 'two'}; + const result = pipe.transform(input); + + // Numeric keys become strings at runtime (Object.keys() always returns + // strings), so `key` should stay `string`, not narrow to `1 | 2`. + const key: string = result[0].key; + + expect(key).toBe('1'); + expect(result).toEqual([ + {key: '1', value: 'one'}, + {key: '2', value: 'two'}, + ]); + }); + it('should accept an object with optional keys', () => { interface MyInterface { one: string;