chore(typings): added typings for minor operators cache, catch, debou… #1352
Conversation
david-driscoll
force-pushed
the
typings/misc
branch
3 times, most recently
from
c816f1c
to
a3c7f91
Compare
| @@ -0,0 +1,10 @@ | |||
| import {Observable, ObservableInput} from '../Observable'; | |||
|
|
|||
| export type _mapProject<T, R> = (value: T, index: number) => R; | |||
There was a problem hiding this comment.
I think these input types could be a separate PR as it's not a dependency for creating the *Signature interfaces.
There was a problem hiding this comment.
It can be, in that case those new PR should be checked in prior to this though.
- is `util' feasible place for these? (I'm also not sure)
- as above comment, does
_needed?
There was a problem hiding this comment.
I tried this by myself and found it's indifferent to have _ or not, by this are actually not being exposed as pubic type definition. If intention is to use idiom of non-exposed export, I'm ok with this. Up to you. :)
There was a problem hiding this comment.
one thing though, as other signature does I'd like to suggest name these such as predicateSignature, etcs to have clarity since it's not actual predicate but signature of predicate. Any suggestions?
There was a problem hiding this comment.
I'll just rip them out for now, just easier than splitting hairs. 👍
david-driscoll
force-pushed
the
typings/misc
branch
2 times, most recently
from
e87f4fd
to
0006170
Compare
| @@ -2,6 +2,6 @@ | |||
| import {Observable} from '../../Observable'; | |||
| import {first} from '../../operator/first'; | |||
|
|
|||
| Observable.prototype.first = first; | |||
| Observable.prototype.first = <any>first; | |||
There was a problem hiding this comment.
Copying over my comment I guess; is this not assignable as is?
There was a problem hiding this comment.
without it there was a compiler error
There was a problem hiding this comment.
Tracing it back, it looks like it's because you have an overload in FirstSignature with a void parameter. I don't think its possible to pass an argument of type void in TypeScript, so that overload can't be called. Changing to any makes it compile again, although I don't understand what use case that second overload satisfies.
There was a problem hiding this comment.
It lets you supply null / undefined and skip the R argument. see example.
Now it does look like that last overload shouldn't be <T | R> but just <R>, I'll try and fix that tomorrow morning.
|
Change looks good to me, might need response for @masaeedu 's comment only. Will leave this until question's fully resolved before check this in. |
david-driscoll
force-pushed
the
typings/misc
branch
from
0006170
to
1fd7911
Compare
|
Fixed up first/last so that the default value is |
|
Will check in this around tomorrow. /cc @masaeedu for visibility to see if there's other suggestions before check this ins. |
| @@ -17,11 +17,18 @@ import {EmptyError} from '../util/EmptyError'; | |||
| * @throws - Throws if no items that match the predicate are emitted by the source Observable. | |||
| */ | |||
| export function last<T, R>(predicate?: (value: T, index: number, source: Observable<T>) => boolean, | |||
| resultSelector?: (value: T, index: number) => R, | |||
| defaultValue?: R): Observable<T> | Observable<R> { | |||
| resultSelector?: (value: T, index: number) => R | void, | |||
There was a problem hiding this comment.
Can this be changed to _mapProject? I think the compiler is interpreting the return type as R | void, rather than the interpreting the whole argument as predicate | void. If this is what it's supposed to be, the last overload in the interface below should be updated from _mapProject.
There was a problem hiding this comment.
I thought projection signature (like _mapProject) was taken out in this PR to be dealt separately, isn't it?
There was a problem hiding this comment.
@kwonoj Whoops, that's right. I was looking at the squashed changes. The fix here would instead be to add parens like so: ((value: T, index: number) => R) | void
There was a problem hiding this comment.
Gotcha, since those to be dealt with projection signature in later, I assume it'd good to kick off this PR to be checked in first - what do you think? @masaeedu
|
LGTM. Some minor quibbles with the function signatures that have to be cast to |
|
Merged with 9b9b2de, appreciate @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. |
…nce, etc
Will need to be rebased after #1351