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

Update filter.ts #5937

Merged
merged 5 commits into from
Jan 26, 2021
Merged

Update filter.ts #5937

merged 5 commits into from
Jan 26, 2021

Conversation

leggechr
Copy link
Contributor

Description:
Remove helpful type at the beginning because it actually was incorrectly returning type never in some cases.

Remove helpful type at the beginning because it actually was incorrectly returning type never in some cases.
Remove filter test
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this change should be accompanied by a failing test that's fixed by the removal of the signature - i.e. something that actually demonstrates the problem.

@MaximSagan
Copy link

I would think that if your predicate unconditionally returns false, never would be the correct type

@cartant
Copy link
Collaborator

cartant commented Dec 17, 2020

I suspect that predicates that return any will match the first signature, but I'd like to see a failing test that verifies this - especially, as these types of signatures are used elsewhere in the lib.

@cartant
Copy link
Collaborator

cartant commented Dec 17, 2020

@leggechr I've added a test that fails if the signature is not removed. If this isn't representative of the code that effected the bug, could you please add a test for that use case, too?

@benlesh This is going to be a problem with any other signatures that also rely upon matching predicates that return false. Any such signatures will need to be removed.

@leggechr
Copy link
Contributor Author

Hey! Yes that is the correct use case. Thanks for updating.

@benlesh
Copy link
Member

benlesh commented Jan 26, 2021

@cartant it looks like this problem may exist with first, last, single, takeWhile, and iif

@benlesh
Copy link
Member

benlesh commented Jan 26, 2021

Related: #5987

@benlesh benlesh merged commit 2c89bb7 into ReactiveX:master Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants