Skip to content

Commit

Permalink
fix(toObservableValue): accommodate all observable inputs (#2471)
Browse files Browse the repository at this point in the history
- 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<X> | Observable<Y> | Observable<Z>`, it's really no different than `Observable<X | Y | Z>`,
  because there's no static/synchronous way to narrow the type of the `Observable` to say, `Observable<X>`
  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.
  • Loading branch information
benlesh authored Apr 3, 2020
1 parent 19f1bda commit 468303a
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 88 deletions.
15 changes: 13 additions & 2 deletions modules/component/spec/core/projections/toObservableValue.spec.ts
Original file line number Diff line number Diff line change
@@ -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<any> = toObservableValue(EMPTY);
expect(isObservable(observable)).toBe(true);
Expand All @@ -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<any> = toObservableValue(set.values());
expect(isObservable(observable)).toBe(true);
});

it('should take undefined', () => {
const observable: Observable<any> = toObservableValue(undefined);
expect(isObservable(observable)).toBe(true);
Expand All @@ -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<any> = toObservableValue(null);
observable.subscribe({
error(e) {
Expand Down
2 changes: 1 addition & 1 deletion modules/component/src/core/cd-aware.abstract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export function createCdAware<U>(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();
Expand Down
33 changes: 4 additions & 29 deletions modules/component/src/core/projections/toObservableValue.ts
Original file line number Diff line number Diff line change
@@ -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<T>(
p: PotentialObservableValue<T>
): Output<T> {
// 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<T>(p)) {
return p;
}

if (isPromiseGuard<T>(p)) {
return from(p);
}

throw new Error(
'Argument not observable. Only null/undefined or Promise/Observable-like values are allowed.'
);
p: ObservableInput<T> | undefined | null
): Observable<T | undefined | null> {
return p == null ? of(p) : from(p);
}
11 changes: 0 additions & 11 deletions modules/component/src/core/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
45 changes: 0 additions & 45 deletions modules/component/src/core/utils/typing.ts

This file was deleted.

0 comments on commit 468303a

Please sign in to comment.