feat(filter): improve type inference for filter(Boolean)

[josepot]

I think that the changes on this PR improve the types of the BooleanConstructor overload of filter

EDIT: I force-pushed after @cartant approved the PR because I realized that I forgot to add -0 to the list of falsy types. Sorry about that! 😬

[cartant]

LGTM

[cartant]

Looking at this again, I think a dtstlint test needs to be added to ensure that the problem mentioned in this comment does not occur: #4959 (comment)

See this comment which refers to the type signature that this PR changes:

rxjs/src/internal/operators/filter.ts

Lines 8 to 9 in adbe65e

	// NOTE(benlesh): T|null|undefined solves the issue discussed here: https://github.com/ReactiveX/rxjs/issues/4959#issuecomment-520629091
	export function filter<T>(predicate: BooleanConstructor): OperatorFunction<T | null | undefined, NonNullable<T>>;

[josepot]

Looking at this again, I think a dtstlint test needs to be added to ensure that the problem mentioned in this comment does not occur: #4959 (comment)

I was under the impression that the current dtslint test already does that.

[benlesh]

I feel like this is something that must exist elsewhere. first() for example. We probably need a type like:

type Truthy<T>  = T extends null | undefined | false | 0 | -0 | 0n | '' ? never : T

[benlesh]

OH NO! haha... This fails and worse, it's typed as Observable<true>... and it's obvious why once you think about it.

  const x = of(false, false, false, false).pipe(filter(Boolean)); // $ExpectType Observable<never>

I'm trying to work on an improvement right now.

[josepot]

right, because false type is boolean, if it had been of(false as const).pipe(filter(Boolean)) then the expected type would have been Observable<never>... I don't know, I won't feel bad if you revert the commit, but IMO that kinda makes sense 🤷

[benlesh]

I'm still thinking about this, really.

[benlesh]

I mean... look at how bananas this is:

type Test<T>  = T extends true ? 'T' : 'F';

function test<T>(value: readonly T[]): Test<T> {
  return null!;
}

const o = test([false, false]); // $ExpectType "F" | "T"   (no, really). 

Same for extends false. I'm sure there's some principled reason for this, but it's baffling to me at the moment.

[josepot]

Yup, fair enough. I agree. Please revert and let's pretend that this never happened 😅 Thanks!

[benlesh]

@josepot Naw.. it's a good change... and it spurred me to do this:

#5842