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

Fix type guard additions #2170

Merged
merged 5 commits into from
Dec 7, 2016
Merged

Fix type guard additions #2170

merged 5 commits into from
Dec 7, 2016

Conversation

rob3c
Copy link
Contributor

@rob3c rob3c commented Dec 3, 2016

Description:
This is a fix for the recently-added type guard support in the filter, find, first and last operator typings. Please see the test cases for details of what's working now, and be sure to let me know if I missed anything this time! ;-)

Related issue (if exists):
Apparently, my previous PR in #2119 is being reverted in #2164 due to the important missed case mentioned in #2163. Unfortunately, I didn't receive any notifications about it, but I'm glad I saw it today while lurking through the repo! :-) Hopefully, this PR should address the problem (which I'm embarrased that I missed), and let us keep the new support without breaking previous behavior.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.686% when pulling d9ed68a on rob3c:fix-type-guard-additions into bd56b3c on ReactiveX:master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 5, 2016

Coverage Status

Coverage remained the same at 97.686% when pulling d9ed68a on rob3c:fix-type-guard-additions into bd56b3c on ReactiveX:master.

@jayphelps
Copy link
Member

Cc/ @david-driscoll can you review?

@david-driscoll
Copy link
Member

I swear I had tried overload ordering the other day to validate if this was possible.

I tried again just now and these changes seem to be working just fine. 👍

cc @saneyuki for an extra set of 👀

@rob3c
Copy link
Contributor Author

rob3c commented Dec 6, 2016

@david-driscoll Yep, the ordering is definitely necessary! It wasn't enough, though, so there are some strategic non-optional function parameters in there, too, to help typescript resolve certain cases.

@tetsuharuohzeki
Copy link
Contributor

@rob3c @david-driscoll

I confirmed that in #2164 we need to write the version which takes predicate returning boolean before to define the one which takes the type guard predicate so that the type inference works correctly for the case of that predicate returning boolean simply. (So we need to define the order of bar<T>() => bar<T, S extends T>())

Is it resolved in this? The current patches are not care about them.

@rob3c
Copy link
Contributor Author

rob3c commented Dec 6, 2016

@saneyuki That's what I originally thought, too.

Surprisingly, it turns out that <T, S extends T> (and <T, S extends T, R> for first and last) needs to be declared before the simpler <T> (and <T, R>) definitions, but it needs to be combined with carefully-defined required/optional parameters, so typescript can distinguish all desired cases. Otherwise, we get the dreaded {} type inferred instead of the actual types for some of them.

@kwonoj
Copy link
Member

kwonoj commented Dec 7, 2016

LGTM

@benlesh benlesh merged commit 5f2e849 into ReactiveX:master Dec 7, 2016
@benlesh
Copy link
Member

benlesh commented Dec 7, 2016

Thanks so much, @rob3c!!

@rob3c rob3c deleted the fix-type-guard-additions branch December 7, 2016 03:58
@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.

7 participants