-
Notifications
You must be signed in to change notification settings - Fork 3k
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
refactor(combineLatest): Improve types #5847
refactor(combineLatest): Improve types #5847
Conversation
benlesh
commented
Oct 22, 2020
- Reduces some type complexity in combineLatest
- Improves types around resultSelectors
- Reduces some type complexity in combineLatest - Improves types around result selectors
@@ -37,31 +37,31 @@ it('should accept union types', () => { | |||
}); | |||
|
|||
it('should accept 1 param and a result selector', () => { | |||
const o = combineLatest(a$, () => new A()); // $ExpectType Observable<A> | |||
const o = combineLatest(a$, (a) => new A()); // $ExpectType Observable<A> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Specifying the parameter here does nothing, ATM, but we could improve the tests by using them:
const o = combineLatest(a$, (a) => [a] as const); // $ExpectType Observable<[A]>
>( | ||
sources: [O1, O2, O3, O4, O5, O6], | ||
export function combineLatest<A extends readonly unknown[], R>( | ||
sources: readonly [...ObservableInputTuple<A>], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: R
is not used in this sig. The return type should be Observable<A>
, AFAICT.
nitpick: Why is the surrounding [... ]
needed? Why not sources: ObservableInputTuple<A>
? This occurs elsewhere, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having used this, I can see, now, that the [... ]
business is needed.
/** @deprecated Use the version that takes an array of Observables instead */ | ||
export function combineLatest<T, R>(source: ObservableInput<T>, resultSelector: (value: T) => R, scheduler?: SchedulerLike): Observable<R>; | ||
/** @deprecated Use the version that takes an array of Observables instead */ | ||
export function combineLatest<T>(source: ObservableInput<T>, scheduler: SchedulerLike): Observable<[T]>; | ||
/** @deprecated Use the version that takes an array of Observables instead */ | ||
export function combineLatest<T>(source: ObservableInput<T>): Observable<[T]>; | ||
|
||
/** @deprecated Passing a scheduler here is deprecated, use {@link subscribeOn} and/or {@link observeOn} instead */ | ||
export function combineLatest<R>( | ||
...observables: Array<ObservableInput<any> | ((...values: Array<any>) => R) | SchedulerLike> | ||
/** @deprecated Use the version that takes an array of Observables instead */ | ||
export function combineLatest<A extends readonly unknown[], R>(...sources: [...ObservableInputTuple<A>, (...args: A) => R]): Observable<R>; | ||
/** @deprecated Use the version that takes an array of Observables instead */ | ||
export function combineLatest<A extends readonly unknown[], R>( | ||
...sources: [...ObservableInputTuple<A>, (...args: A) => R, SchedulerLike] | ||
): Observable<R>; | ||
/** @deprecated Use the version that takes an array of Observables instead */ | ||
export function combineLatest<A extends readonly unknown[]>(...sources: [...ObservableInputTuple<A>, SchedulerLike]): Observable<A>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why does this use an approach that differs from what's done with merge
? Does this behave a little better? Are there tests to show this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>( | ||
sources: [O1, O2, O3, O4, O5, O6], | ||
export function combineLatest<A extends readonly unknown[], R>( | ||
sources: readonly [...ObservableInputTuple<A>], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOO is readonly
needed here? Doesn't make a difference IMO, purely curious
Closing this 'cause the |