From 4dfe0fa068007c28ca3b4a1476ed4ba1d2d86064 Mon Sep 17 00:00:00 2001 From: Andrea Canciani Date: Fri, 27 Mar 2020 11:47:59 +0100 Subject: [PATCH] fix(common): correct and simplify typing of `KeyValuePipe` (#37447) As shown in the tests, `KeyValuePipe.transform` can accept `undefined`, in which case it always returns `null`. Additionally, the typing for `string` keys can be made generic, so the comparison function is only required to accept the relevant cases. Finally, the typing for `number` records now shows that the comparison function and the result entries will actually receive the string version of the numeric keys, just as shown in the tests. BREAKING CHANGE: The typing of the `keyvalue` pipe has been fixed to report that for input objects that have `number` keys, the result will contain the string representation of the keys. This was already the case and the code has simply been updated to reflect this. Please update the consumers of the pipe output if they were relying on the incorrect types. Note that this does not affect use cases where the input values are `Map`s, so if you need to preserve `number`s, this is an effective way. PR Close #37447 --- goldens/public-api/common/common.d.ts | 20 +++------ packages/common/src/pipes/keyvalue_pipe.ts | 42 ++++++++++--------- .../common/test/pipes/keyvalue_pipe_spec.ts | 9 ++-- 3 files changed, 34 insertions(+), 37 deletions(-) diff --git a/goldens/public-api/common/common.d.ts b/goldens/public-api/common/common.d.ts index df57950d36..89ac6ad48e 100644 --- a/goldens/public-api/common/common.d.ts +++ b/goldens/public-api/common/common.d.ts @@ -140,21 +140,13 @@ export declare interface KeyValue { export declare class KeyValuePipe implements PipeTransform { constructor(differs: KeyValueDiffers); - transform(input: null, compareFn?: (a: KeyValue, b: KeyValue) => number): null; - transform(input: { - [key: string]: V; - } | ReadonlyMap, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; - transform(input: { - [key: string]: V; - } | ReadonlyMap | null, compareFn?: (a: KeyValue, b: KeyValue) => number): Array> | null; - transform(input: { - [key: number]: V; - } | ReadonlyMap, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; - transform(input: { - [key: number]: V; - } | ReadonlyMap | null, compareFn?: (a: KeyValue, b: KeyValue) => number): Array> | null; transform(input: ReadonlyMap, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; - transform(input: ReadonlyMap | null, compareFn?: (a: KeyValue, b: KeyValue) => number): Array> | null; + transform(input: Record, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; + transform(input: Record | ReadonlyMap, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; + transform(input: null | undefined, compareFn?: (a: KeyValue, b: KeyValue) => number): null; + transform(input: ReadonlyMap | null | undefined, compareFn?: (a: KeyValue, b: KeyValue) => number): Array> | null; + transform(input: Record | null | undefined, compareFn?: (a: KeyValue, b: KeyValue) => number): Array> | null; + transform(input: Record | ReadonlyMap | null | undefined, compareFn?: (a: KeyValue, b: KeyValue) => number): Array> | null; } export declare class Location { diff --git a/packages/common/src/pipes/keyvalue_pipe.ts b/packages/common/src/pipes/keyvalue_pipe.ts index e81fff1326..163b83f3e6 100644 --- a/packages/common/src/pipes/keyvalue_pipe.ts +++ b/packages/common/src/pipes/keyvalue_pipe.ts @@ -50,31 +50,35 @@ export class KeyValuePipe implements PipeTransform { private differ!: KeyValueDiffer; private keyValues: Array> = []; - transform(input: null, compareFn?: (a: KeyValue, b: KeyValue) => number): null; - transform( - input: {[key: string]: V}|ReadonlyMap, - compareFn?: (a: KeyValue, b: KeyValue) => number): - Array>; - transform( - input: {[key: string]: V}|ReadonlyMap|null, - compareFn?: (a: KeyValue, b: KeyValue) => number): - Array>|null; - transform( - input: {[key: number]: V}|ReadonlyMap, - compareFn?: (a: KeyValue, b: KeyValue) => number): - Array>; - transform( - input: {[key: number]: V}|ReadonlyMap|null, - compareFn?: (a: KeyValue, b: KeyValue) => number): - Array>|null; + /* + * NOTE: when the `input` value is a simple Record object, the keys are extracted with + * Object.keys(). This means that even if the `input` type is Record the keys are + * compared/returned as `string`s. + */ transform( input: ReadonlyMap, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; + transform( + input: Record, compareFn?: (a: KeyValue, b: KeyValue) => number): + Array>; + transform( + input: Record|ReadonlyMap, + compareFn?: (a: KeyValue, b: KeyValue) => number): Array>; + transform( + input: null|undefined, + compareFn?: (a: KeyValue, b: KeyValue) => number): null; transform( - input: ReadonlyMap|null, + input: ReadonlyMap|null|undefined, + compareFn?: (a: KeyValue, b: KeyValue) => number): Array>|null; + transform( + input: Record|null|undefined, + compareFn?: (a: KeyValue, b: KeyValue) => number): + Array>|null; + transform( + input: Record|ReadonlyMap|null|undefined, compareFn?: (a: KeyValue, b: KeyValue) => number): Array>|null; transform( - input: null|{[key: string]: V, [key: number]: V}|ReadonlyMap, + input: undefined|null|{[key: string]: V, [key: number]: V}|ReadonlyMap, compareFn: (a: KeyValue, b: KeyValue) => number = defaultComparator): Array>|null { if (!input || (!(input instanceof Map) && typeof input !== 'object')) { diff --git a/packages/common/test/pipes/keyvalue_pipe_spec.ts b/packages/common/test/pipes/keyvalue_pipe_spec.ts index b6741b284a..2b9fb49533 100644 --- a/packages/common/test/pipes/keyvalue_pipe_spec.ts +++ b/packages/common/test/pipes/keyvalue_pipe_spec.ts @@ -17,12 +17,12 @@ describe('KeyValuePipe', () => { }); it('should return null when given undefined', () => { const pipe = new KeyValuePipe(defaultKeyValueDiffers); - expect(pipe.transform(undefined as any)).toEqual(null); + expect(pipe.transform(undefined)).toEqual(null); }); it('should return null for an unsupported type', () => { const pipe = new KeyValuePipe(defaultKeyValueDiffers); const fn = () => {}; - expect(pipe.transform(fn as any)).toEqual(null); + expect(pipe.transform(fn as any as null)).toEqual(null); }); describe('object dictionary', () => { it('should return empty array of an empty dictionary', () => { @@ -98,8 +98,9 @@ describe('KeyValuePipe', () => { }); it('should order by numerical and alpha', () => { const pipe = new KeyValuePipe(defaultKeyValueDiffers); - const input = [[2, 1], [1, 1], ['b', 1], [0, 1], [3, 1], ['a', 1]]; - expect(pipe.transform(new Map(input as any))).toEqual([ + const input = + [[2, 1], [1, 1], ['b', 1], [0, 1], [3, 1], ['a', 1]] as Array<[number | string, number]>; + expect(pipe.transform(new Map(input))).toEqual([ {key: 0, value: 1}, {key: 1, value: 1}, {key: 2, value: 1}, {key: 3, value: 1}, {key: 'a', value: 1}, {key: 'b', value: 1} ]);