Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(toObservableValue): accommodate all observable inputs #2471

Merged
merged 1 commit into from
Apr 3, 2020

Conversation

benlesh
Copy link
Contributor

@benlesh benlesh commented Apr 3, 2020

  • 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.

- 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.
@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 3, 2020

Preview docs changes for 7494cef at https://previews.ngrx.io/pr2471-7494cef/

@brandonroberts brandonroberts merged commit 468303a into ngrx:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants