-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(filter): Observable<T>.filter() can take a type guard as the predicate function #1936
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
Conversation
Let me preface this with, awesome job, this is something I wouldn't have thought of directly, but it does make it much easier to flow types, if you have to deal with unions 👏. Only issue I have is that I think this applies to more operators than just filter. I feel like this would benefit....
It may even also apply to |
Agreeing with @david-driscoll , I think this change's good and would like to expand to other possibly supported operators as well. |
okay. I'll update this pull request. |
f3c33cd
to
4494973
Compare
updated :) |
LGTM 👍 |
<R>(predicate?: (value: T, index: number, source: Observable<T>) => boolean, resultSelector?: (value: T, index: number) => R, | ||
defaultValue?: R): Observable<R>; | ||
// We can cast `T -> R` in it if we pass the result selector. |
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's just nitpicking, but I'm feeling this'd be better to be interface level comment (@ line 61) - what do you think?
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.
Ok. I'll change to it.
LGTM, only one nitpicking comment about comment location. |
4494973
to
76a8a57
Compare
@kwonoj I just updated & fixed what your pointed. |
I just asked about how to flow types through Observable chains in this typescript issue: microsoft/TypeScript#7657 (comment) It'd be really nice to avoid what seems like a bunch of unnecessary casting, but I'm not sure if it's a typescript limitation or if it's addressable via better type definitions. |
Just a quick update: I found a way to rewrite the type definitions to not require all of the casting above. I'm creating a pull request now... |
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. |
Description
Before
By the previous code base, we need to use
filter()
+map()
After
By this change, we can write this code:
Related issue (if exists)
microsoft/TypeScript#10027