From 468303af77b6efcb5760bc3f766ad7326103bead Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Fri, 3 Apr 2020 16:13:26 -0500 Subject: [PATCH] fix(toObservableValue): accommodate all observable inputs (#2471) - Switches toObservableValue to just use rxjs `from`. This will ensure it works with all types that `rxjs` is compatable with. Also, the checks in `toObservableValue` were redundant if `from` was used. For example, if `x` is `Observable` from rxjs, then `from(x) === x` would be `true`. `from` does all of the necessary checks. Also, `from` will do the work of figuring out of it happens to be an observable that has interop with RxJS, or is an observable from another instance of rxjs (in node_modules, Node scenario). - Also `x == null` will only return `true` if `x` is `undefined` or `null`. - Fixes the return types of `toObservableValue` to be more true to form. If you return `Observable | Observable | Observable`, it's really no different than `Observable`, because there's no static/synchronous way to narrow the type of the `Observable` to say, `Observable` in either scenario. - Removes unecessary types in favor of more obvious explicit types - Removes superfluous isObservable-type assert function - Removes unused assertion methods. Not sure what those were used for, as they were exported, but two were ill-advised, as they were doing type-narrowing by iterating over an array, making it an O(n) operation just to narrow a type. --- .../projections/toObservableValue.spec.ts | 15 ++++++- .../component/src/core/cd-aware.abstract.ts | 2 +- .../src/core/projections/toObservableValue.ts | 33 ++------------ modules/component/src/core/utils/index.ts | 11 ----- modules/component/src/core/utils/typing.ts | 45 ------------------- 5 files changed, 18 insertions(+), 88 deletions(-) delete mode 100644 modules/component/src/core/utils/typing.ts diff --git a/modules/component/spec/core/projections/toObservableValue.spec.ts b/modules/component/spec/core/projections/toObservableValue.spec.ts index 5d85a19018..13f15827a1 100644 --- a/modules/component/spec/core/projections/toObservableValue.spec.ts +++ b/modules/component/spec/core/projections/toObservableValue.spec.ts @@ -1,8 +1,11 @@ -import { EMPTY, isObservable, Observable, of } from 'rxjs'; +import { EMPTY, isObservable, Observable } from 'rxjs'; import { toObservableValue } from '../../../src/core/projections'; describe('toObservableValue', () => { describe('used as RxJS creation function', () => { + // NOTE: (benlesh) These tests are probably all redundant, as you're just + // testing `rxjs` from in every case but `null` and `undefined`. + it('should take observables', () => { const observable: Observable = toObservableValue(EMPTY); expect(isObservable(observable)).toBe(true); @@ -15,6 +18,12 @@ describe('toObservableValue', () => { expect(isObservable(observable)).toBe(true); }); + it('should take an iterable', () => { + const set = new Set([1, 2, 3]); + const observable: Observable = toObservableValue(set.values()); + expect(isObservable(observable)).toBe(true); + }); + it('should take undefined', () => { const observable: Observable = toObservableValue(undefined); expect(isObservable(observable)).toBe(true); @@ -25,7 +34,9 @@ describe('toObservableValue', () => { expect(isObservable(observable)).toBe(true); }); - it('throw if no observable, promise, undefined or null is passed', () => { + // NOTE: (benlesh) - AFIACT this test would never have passed with the existing code + // `toObservableValue(null)` was made to return `of(null)` + xit('throw if no observable, promise, undefined or null is passed', () => { const observable: Observable = toObservableValue(null); observable.subscribe({ error(e) { diff --git a/modules/component/src/core/cd-aware.abstract.ts b/modules/component/src/core/cd-aware.abstract.ts index a398902ae2..bc74cd622d 100644 --- a/modules/component/src/core/cd-aware.abstract.ts +++ b/modules/component/src/core/cd-aware.abstract.ts @@ -61,7 +61,7 @@ export function createCdAware(cfg: { // Ignore potential observables of the same instances distinctUntilChanged(), // Try to convert it to values, throw if not possible - map(v => toObservableValue(v)), + map(toObservableValue), tap((v: any) => { cfg.resetContextObserver.next(v); cfg.work(); diff --git a/modules/component/src/core/projections/toObservableValue.ts b/modules/component/src/core/projections/toObservableValue.ts index 4f45ca06ee..16c83ff5f5 100644 --- a/modules/component/src/core/projections/toObservableValue.ts +++ b/modules/component/src/core/projections/toObservableValue.ts @@ -1,32 +1,7 @@ -import { from, of } from 'rxjs'; -import { - isObservableGuard, - isPromiseGuard, - PotentialObservableValue, - Output, -} from '../utils'; +import { from, of, Observable, ObservableInput } from 'rxjs'; export function toObservableValue( - p: PotentialObservableValue -): Output { - // Comparing to the literal null value with the == operator covers both null and undefined values. - if (p === null) { - return of(p); - } - - if (p === undefined) { - return of(p); - } - - if (isObservableGuard(p)) { - return p; - } - - if (isPromiseGuard(p)) { - return from(p); - } - - throw new Error( - 'Argument not observable. Only null/undefined or Promise/Observable-like values are allowed.' - ); + p: ObservableInput | undefined | null +): Observable { + return p == null ? of(p) : from(p); } diff --git a/modules/component/src/core/utils/index.ts b/modules/component/src/core/utils/index.ts index e86c97f344..23708a2180 100644 --- a/modules/component/src/core/utils/index.ts +++ b/modules/component/src/core/utils/index.ts @@ -2,14 +2,3 @@ export { getChangeDetectionHandler } from './get-change-detection-handling'; export { getGlobalThis } from './get-global-this'; export { isIvy } from './is-ivy'; export { hasZone } from './has-zone'; - -export { - isDefinedGuard, - isIterableGuard, - isObservableGuard, - isOperateFnArrayGuard, - isPromiseGuard, - isStringArrayGuard, - PotentialObservableValue, - Output, -} from './typing'; diff --git a/modules/component/src/core/utils/typing.ts b/modules/component/src/core/utils/typing.ts deleted file mode 100644 index 48fa38e950..0000000000 --- a/modules/component/src/core/utils/typing.ts +++ /dev/null @@ -1,45 +0,0 @@ -import { isObservable, Observable, OperatorFunction } from 'rxjs'; -export type PotentialObservableValue = - | Observable - | Promise - | undefined - | null; -export type Output = - | Observable - | Observable - | Observable; - -export function isPromiseGuard(value: any): value is Promise { - return ( - !!value && - typeof (value as any).subscribe !== 'function' && - typeof (value as any).then === 'function' - ); -} - -export function isObservableGuard( - potentialObservable: any -): potentialObservable is Observable { - return isObservable(potentialObservable); -} - -export function isOperateFnArrayGuard( - op: any[] -): op is OperatorFunction[] { - return op.every((i: any) => typeof i !== 'string'); -} - -export function isStringArrayGuard(op: any[]): op is string[] { - return op.every((i: any) => typeof i !== 'string'); -} - -export function isDefinedGuard(opr: any): opr is T { - return !!opr; -} - -export function isIterableGuard(obj: any): obj is Array { - if (obj === undefined) { - return false; - } - return typeof (obj as any)[Symbol.iterator] === 'function'; -}