-
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
chore(typings): added type signatures for concat/exhaust/merge/switch #1351
Conversation
e19cecf
to
d5c6dc7
Compare
private ish: Observable<R> | Promise<R>, | ||
private resultSelector?: (outerValue: T, innerValue: R, outerIndex: number, innerIndex: number) => R2, | ||
constructor(destination: Subscriber<R>, | ||
private ish: Observable<I> | Promise<I>, |
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.
can this be ObservableOrPromise<T>
?
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.
it can, updated.
bad1781
to
51bcb7f
Compare
Change looks good to me. Will try to check in after #1352. |
this may need a rebase, so I'll keep an eye on on #1352, and get that done asap |
@david-driscoll seems it requires rebase as you've expected. :) In general I think change's ok, cc @masaeedu for visibility just in case. |
resultSelector?: (outerValue: T, innerValue: R, outerIndex: number, innerIndex: number) => R2 | number, | ||
concurrent: number = Number.POSITIVE_INFINITY): Observable<R2> { | ||
export function mergeMap<T, I, R>(project: (value: T, index: number) => ObservableInput<I>, | ||
resultSelector?: (outerValue: T, innerValue: I, outerIndex: number, innerIndex: number) => R | number, |
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.
Grouping issue again; I think this should be ((outerValue: T, innerValue: I, outerIndex: number, innerIndex: number) => R) | number
, which makes mergeMap
assignable to its signature.
@kwonoj Similar non-blocking quibbles as the last one. LGTM. |
Cool, let me kick off this once it's rebased. |
51bcb7f
to
212084d
Compare
@david-driscoll seems there was some failure with type definition update? |
212084d
to
9eb988e
Compare
also adds signatures for the "All", "Map", and "MapTo" variants.
9eb988e
to
1933774
Compare
Yeah, missed that. Updated now. |
Merged with 1933774, thanks @david-driscoll ! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
also adds signatures for the "All", "Map", and "MapTo" variants.