From 7b2aac97df63717fd2dfbb81903d12221ae29ba1 Mon Sep 17 00:00:00 2001 From: Andrea Canciani Date: Fri, 27 Mar 2020 11:41:50 +0100 Subject: [PATCH] feat(common): stricter types for number pipes (#37447) Make typing of number pipes stricter to catch some misuses (such as passing an Observable or an array) at compile time. BREAKING CHANGE: The signatures of the number pipes now explicitly state which types are accepted. This should only cause issues in corner cases, as any other values would result in runtime exceptions. PR Close #37447 --- goldens/public-api/common/common.d.ts | 12 ++++-- packages/common/src/pipes/number_pipe.ts | 36 ++++++++++++---- .../common/test/pipes/number_pipe_spec.ts | 42 +++++++++++++++++-- 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/goldens/public-api/common/common.d.ts b/goldens/public-api/common/common.d.ts index 418fecafd2..12574a6997 100644 --- a/goldens/public-api/common/common.d.ts +++ b/goldens/public-api/common/common.d.ts @@ -13,7 +13,9 @@ export declare class CommonModule { export declare class CurrencyPipe implements PipeTransform { constructor(_locale: string, _defaultCurrencyCode?: string); - transform(value: any, currencyCode?: string, display?: 'code' | 'symbol' | 'symbol-narrow' | string | boolean, digitsInfo?: string, locale?: string): string | null; + transform(value: number | string, currencyCode?: string, display?: 'code' | 'symbol' | 'symbol-narrow' | string | boolean, digitsInfo?: string, locale?: string): string | null; + transform(value: null | undefined, currencyCode?: string, display?: 'code' | 'symbol' | 'symbol-narrow' | string | boolean, digitsInfo?: string, locale?: string): null; + transform(value: number | string | null | undefined, currencyCode?: string, display?: 'code' | 'symbol' | 'symbol-narrow' | string | boolean, digitsInfo?: string, locale?: string): string | null; } export declare class DatePipe implements PipeTransform { @@ -25,7 +27,9 @@ export declare class DatePipe implements PipeTransform { export declare class DecimalPipe implements PipeTransform { constructor(_locale: string); - transform(value: any, digitsInfo?: string, locale?: string): string | null; + transform(value: number | string, digitsInfo?: string, locale?: string): string | null; + transform(value: null | undefined, digitsInfo?: string, locale?: string): null; + transform(value: number | string | null | undefined, digitsInfo?: string, locale?: string): string | null; } export declare const DOCUMENT: InjectionToken; @@ -342,7 +346,9 @@ export declare class PathLocationStrategy extends LocationStrategy { export declare class PercentPipe implements PipeTransform { constructor(_locale: string); - transform(value: any, digitsInfo?: string, locale?: string): string | null; + transform(value: number | string, digitsInfo?: string, locale?: string): string | null; + transform(value: null | undefined, digitsInfo?: string, locale?: string): null; + transform(value: number | string | null | undefined, digitsInfo?: string, locale?: string): string | null; } export declare abstract class PlatformLocation { diff --git a/packages/common/src/pipes/number_pipe.ts b/packages/common/src/pipes/number_pipe.ts index 4931e7aa0c..edc6d08094 100644 --- a/packages/common/src/pipes/number_pipe.ts +++ b/packages/common/src/pipes/number_pipe.ts @@ -67,8 +67,12 @@ export class DecimalPipe implements PipeTransform { * When not supplied, uses the value of `LOCALE_ID`, which is `en-US` by default. * See [Setting your app locale](guide/i18n#setting-up-the-locale-of-your-app). */ - transform(value: any, digitsInfo?: string, locale?: string): string|null { - if (isEmpty(value)) return null; + transform(value: number|string, digitsInfo?: string, locale?: string): string|null; + transform(value: null|undefined, digitsInfo?: string, locale?: string): null; + transform(value: number|string|null|undefined, digitsInfo?: string, locale?: string): string|null; + transform(value: number|string|null|undefined, digitsInfo?: string, locale?: string): string + |null { + if (!isValue(value)) return null; locale = locale || this._locale; @@ -121,8 +125,12 @@ export class PercentPipe implements PipeTransform { * When not supplied, uses the value of `LOCALE_ID`, which is `en-US` by default. * See [Setting your app locale](guide/i18n#setting-up-the-locale-of-your-app). */ - transform(value: any, digitsInfo?: string, locale?: string): string|null { - if (isEmpty(value)) return null; + transform(value: number|string, digitsInfo?: string, locale?: string): string|null; + transform(value: null|undefined, digitsInfo?: string, locale?: string): null; + transform(value: number|string|null|undefined, digitsInfo?: string, locale?: string): string|null; + transform(value: number|string|null|undefined, digitsInfo?: string, locale?: string): string + |null { + if (!isValue(value)) return null; locale = locale || this._locale; try { const num = strToNumber(value); @@ -213,10 +221,22 @@ export class CurrencyPipe implements PipeTransform { * See [Setting your app locale](guide/i18n#setting-up-the-locale-of-your-app). */ transform( - value: any, currencyCode?: string, + value: number|string, currencyCode?: string, + display?: 'code'|'symbol'|'symbol-narrow'|string|boolean, digitsInfo?: string, + locale?: string): string|null; + transform( + value: null|undefined, currencyCode?: string, + display?: 'code'|'symbol'|'symbol-narrow'|string|boolean, digitsInfo?: string, + locale?: string): null; + transform( + value: number|string|null|undefined, currencyCode?: string, + display?: 'code'|'symbol'|'symbol-narrow'|string|boolean, digitsInfo?: string, + locale?: string): string|null; + transform( + value: number|string|null|undefined, currencyCode?: string, display: 'code'|'symbol'|'symbol-narrow'|string|boolean = 'symbol', digitsInfo?: string, locale?: string): string|null { - if (isEmpty(value)) return null; + if (!isValue(value)) return null; locale = locale || this._locale; @@ -246,8 +266,8 @@ export class CurrencyPipe implements PipeTransform { } } -function isEmpty(value: any): boolean { - return value == null || value === '' || value !== value; +function isValue(value: number|string|null|undefined): value is number|string { + return !(value == null || value === '' || value !== value); } /** diff --git a/packages/common/test/pipes/number_pipe_spec.ts b/packages/common/test/pipes/number_pipe_spec.ts index f0210677a0..249cc57eee 100644 --- a/packages/common/test/pipes/number_pipe_spec.ts +++ b/packages/common/test/pipes/number_pipe_spec.ts @@ -50,8 +50,20 @@ import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testin expect(pipe.transform('1.1234')).toEqual('1.123'); }); + it('should return null for NaN', () => { + expect(pipe.transform(Number.NaN)).toEqual(null); + }); + + it('should return null for null', () => { + expect(pipe.transform(null)).toEqual(null); + }); + + it('should return null for undefined', () => { + expect(pipe.transform(undefined)).toEqual(null); + }); + it('should not support other objects', () => { - expect(() => pipe.transform({})) + expect(() => pipe.transform({} as any)) .toThrowError( `InvalidPipeArgument: '[object Object] is not a number' for pipe 'DecimalPipe'`); expect(() => pipe.transform('123abc')) @@ -82,8 +94,20 @@ import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testin expect(pipe.transform(12.3456, '0.0-10')).toEqual('1,234.56%'); }); + it('should return null for NaN', () => { + expect(pipe.transform(Number.NaN)).toEqual(null); + }); + + it('should return null for null', () => { + expect(pipe.transform(null)).toEqual(null); + }); + + it('should return null for undefined', () => { + expect(pipe.transform(undefined)).toEqual(null); + }); + it('should not support other objects', () => { - expect(() => pipe.transform({})) + expect(() => pipe.transform({} as any)) .toThrowError( `InvalidPipeArgument: '[object Object] is not a number' for pipe 'PercentPipe'`); }); @@ -125,8 +149,20 @@ import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testin expect(pipe.transform(5.1234, 'USD', 'Custom name')).toEqual('Custom name5.12'); }); + it('should return null for NaN', () => { + expect(pipe.transform(Number.NaN)).toEqual(null); + }); + + it('should return null for null', () => { + expect(pipe.transform(null)).toEqual(null); + }); + + it('should return null for undefined', () => { + expect(pipe.transform(undefined)).toEqual(null); + }); + it('should not support other objects', () => { - expect(() => pipe.transform({})) + expect(() => pipe.transform({} as any)) .toThrowError( `InvalidPipeArgument: '[object Object] is not a number' for pipe 'CurrencyPipe'`); });