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

feat(filter): Observable<T>.filter() can take a type guard as the predicate function #1936

Merged
merged 4 commits into from
Sep 16, 2016

Conversation

tetsuharuohzeki
Copy link
Contributor

Description

Before

By the previous code base, we need to use filter() + map()

interface SuperClass {}
interface DerivedClass extends SuperClass {}

const bar: Observable<SuperClass> = someobservable;
const derived: Observable<DerivedClass> = bar.filter((v): boolean => {
    // some type guard logic
  }).map<DerivedClass>((v) => v as DerivedClass);

After

By this change, we can write this code:

const bar: Observable<SuperClass> = someobservable;
const derived: Observable<DerivedClass> = bar.filter<DerivedClass>((v): v is DerivedClass => {
  // some type guard logic
});

Related issue (if exists)

microsoft/TypeScript#10027

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.129% when pulling f3c33cd on saneyuki:filter-typing into d876a35 on ReactiveX:master.

@david-driscoll
Copy link
Member

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

  • filter
  • find
  • first
  • last

cc @Blesh @staltz @kwonoj

It may even also apply to partition (although we can only narrow the positive result type).

@kwonoj
Copy link
Member

kwonoj commented Sep 14, 2016

Agreeing with @david-driscoll , I think this change's good and would like to expand to other possibly supported operators as well.

@tetsuharuohzeki
Copy link
Contributor Author

okay. I'll update this pull request.

@tetsuharuohzeki
Copy link
Contributor Author

okay. I'll update this pull request.

updated :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.129% when pulling 4494973 on saneyuki:filter-typing into 1b716ff on ReactiveX:master.

@david-driscoll
Copy link
Member

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.
Copy link
Member

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?

Copy link
Contributor Author

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.

@kwonoj
Copy link
Member

kwonoj commented Sep 14, 2016

LGTM, only one nitpicking comment about comment location.

@tetsuharuohzeki
Copy link
Contributor Author

@kwonoj I just updated & fixed what your pointed.

@coveralls
Copy link

coveralls commented Sep 16, 2016

Coverage Status

Coverage remained the same at 97.128% when pulling 76a8a57 on saneyuki:filter-typing into 36c7676 on ReactiveX:master.

@kwonoj kwonoj merged commit e91d113 into ReactiveX:master Sep 16, 2016
@tetsuharuohzeki tetsuharuohzeki deleted the filter-typing branch September 17, 2016 17:46
@rob3c
Copy link
Contributor

rob3c commented Nov 4, 2016

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.

@rob3c
Copy link
Contributor

rob3c commented Nov 7, 2016

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

@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants