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

Add BooleanConstructor as an overload to .filter to allow for easy type predicate filtering #50387

Open
mattpocock opened this issue Aug 21, 2022 · 12 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@mattpocock
Copy link

lib Update Request

Configuration Check

My compilation target is ES2015 and my lib is the default.

Missing / Incorrect Definition

I would love to add an overload to Array.filter which takes in a common use case of passing a Boolean directly to .filter. This would offer a simple, intuitive workaround for a common pain point with .filter.

This could be achieved via an overload adding BooleanConstructor, and a type predicate.

https://www.typescriptlang.org/play?#code/PTAEHUEsBcAtTgU1AewG6IE4BsUEMATAWACgBjFAOwGdpQ9NNQBeUAbQCIBGDgGlACulAogBmkSogIBdANykQoJaAB6AflKkJ0LKLxlkAQUZ4AngB4AKgD5QAb1JLx2HZgAUAB0xTIZPDoAuUAAhFBRsRDxKAGEqWkwBMmgUTABKIIA5KgAxPGxqCxs2OVIAX00SaFMPZCzKXPzC21ZLUEQADx1halAABlAAH1AAcmHB0EoBbGxxoRFxSQJQNQnEDCYgy3kSUgoaOmdXKWMmVgZMADpDrDdQ8MjKVO3FZXUgA

Sample Code

const arr = ['1', undefined];

const filteredArr = arr.filter(Boolean);

filteredArr should be string[], not (string | undefined)[].

@acutmore
Copy link
Contributor

acutmore commented Aug 21, 2022

Related: #16655 “Boolean() cannot be used to perform a null check”

@lf-novelt
Copy link

lf-novelt commented Aug 21, 2022

A bit convoluted to understand for most people, isn't it?
It looks more like a JS quirks that TS was supposed to prevent than beautiful TS code

@ghoullier
Copy link

see #50377

@mattpocock
Copy link
Author

@ghoullier Aha, didn't see that @orta had already tried it.

LMK how I can help.

@mattpocock
Copy link
Author

mattpocock commented Aug 21, 2022

A bit convoluted to understand for most people, isn't it? It looks more like a JS quirks that TS was supposed to prevent than beautiful TS code

@lf-novelt Could you clarify what you mean? Which bits are convoluted?

The point of intermediate-advanced TS is to try to make the beginner's TS life easier while staying out of the way. I'd argue that this overload does that very well. .filter(Boolean) is a common pattern at all levels of JS, and using it to make TS's inference smarter benefits everyone.

@lf-novelt
Copy link

Indeed this syntax is a cool trick but not obvious to understand at first glance. I don't mind TS supporting it but would not push for this example as a "go to" to filter nulls or undefined from an array.
As some wise man said, you only write code once, but read it many times, so I would write code that's easy to read even if it costs me a few more keystrokes.

And arr.filter(i => i) is even shorter.

But I still think arr.filter(i => i !== undefined && i !== null) is more explicit (and it does not filter out empty string, which is clear)

@MartinJohns
Copy link
Contributor

Duplicate of #16655.

@mattpocock
Copy link
Author

@MartinJohns I don't think it is, it's a separate suggestion which is of much smaller impact.

@mattpocock
Copy link
Author

In fact, you could consider this issue just a moving of this great suggestion into its own issue.

@MartinJohns
Copy link
Contributor

MartinJohns commented Aug 21, 2022

True, it's only a partial duplicate, but your use case is the main one mentioned in #16655 and most of its duplicates.

But I said my bit and I'll drop out of this conversation. Considering the unnatural amount of upvotes this issue already has it's clear that it has been promoted somewhere else.

But just FYI, this suggestion is already being tried out: #50377

@yanickrochon
Copy link

This fails and it shouldn't.

const a = [] as InputsDataType[] | string[] | AppButtonType[] | CustomButtonT[];

const testFilter = (data: InputsDataType): data is InputsDataType => {
  return true;
}

a.filter(testFilter);

@danvk
Copy link
Contributor

danvk commented Apr 4, 2024

After #57465 there's less need for this. You can write arr.filter(x => !!x) instead of arr.filter(Boolean) and it will work for object types, where there are no footguns around 0 and "".

That being said, there was a lot of confusion on Twitter about whether filter(Boolean) will now work. The answer is no. I spent some time trying to fix this in a few different ways. I wasn't able to, but I learned enough along the way that I thought it would be useful to dump state.

Approach 1: add an overload for filter(BooleanConstructor) aka the ts-reset approach

Here's one version of this that specializes on this to restrict to arrays of objects:

diff --git a/src/lib/es5.d.ts b/src/lib/es5.d.ts
index e404df509a..3bbf59bbba 100644
--- a/src/lib/es5.d.ts
+++ b/src/lib/es5.d.ts
@@ -1252,6 +1252,12 @@ interface ReadonlyArray<T> {
      * @param thisArg An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value.
      */
     map<U>(callbackfn: (value: T, index: number, array: readonly T[]) => U, thisArg?: any): U[];
+    filter(this: ReadonlyArray<object | null | undefined>, predicate: BooleanConstructor, thisArg?: any): (T & {})[];
     /**
      * Returns the elements of an array that meet the condition specified in a callback function.
      * @param predicate A function that accepts up to three arguments. The filter method calls the predicate function one time for each element in the array.
@@ -1443,6 +1449,12 @@ interface Array<T> {
      * @param thisArg An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value.
      */
     map<U>(callbackfn: (value: T, index: number, array: T[]) => U, thisArg?: any): U[];
+    filter(this: Array<object | null | undefined>, predicate: BooleanConstructor, thisArg?: any): (T & {})[];
     /**
      * Returns the elements of an array that meet the condition specified in a callback function.
      * @param predicate A function that accepts up to three arguments. The filter method calls the predicate function one time for each element in the array.

Here's another that uses a conditional type to similar ends:

diff --git a/src/lib/es5.d.ts b/src/lib/es5.d.ts
index e404df509a..eaabc04b1b 100644
--- a/src/lib/es5.d.ts
+++ b/src/lib/es5.d.ts
@@ -1258,6 +1258,12 @@ interface ReadonlyArray<T> {
      * @param thisArg An object to which the this keyword can refer in the predicate function. If thisArg is omitted, undefined is used as the this value.
      */
     filter<S extends T>(predicate: (value: T, index: number, array: readonly T[]) => value is S, thisArg?: any): S[];
+    filter(predicate: BooleanConstructor, thisArg?: any): [T] extends ([object|null|undefined]) ? (T & {})[] : T[];
     /**
      * Returns the elements of an array that meet the condition specified in a callback function.
      * @param predicate A function that accepts up to three arguments. The filter method calls the predicate function one time for each element in the array.
@@ -1449,6 +1455,12 @@ interface Array<T> {
      * @param thisArg An object to which the this keyword can refer in the predicate function. If thisArg is omitted, undefined is used as the this value.
      */
     filter<S extends T>(predicate: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[];
+    filter(predicate: BooleanConstructor, thisArg?: any): [T] extends ([object|null|undefined]) ? (T & {})[] : T[];
     /**
      * Returns the elements of an array that meet the condition specified in a callback function.
      * @param predicate A function that accepts up to three arguments. The filter method calls the predicate function one time for each element in the array.

The problem with both of these is that they fundamentally clash with the fallback logic that was introduced for (A[] | B[]).filter in #53489. When you call a method on (A[] | B[]) and TS can't find any candidate overloads, it takes a look at (A | B)[] instead. Once you introduce these BooleanConstructor overloads, it does find a candidate overload. So the fallback logic doesn't run and you get cryptic errors for calls like this:

([] as Fizz[] | Buzz[]).filter(item => item.id < 5);
//                             ~~~~~~~~~~~~~~~~~~~
// Argument of type '(item: any) => boolean' is not assignable to parameter of type 'BooleanConstructor'.

I don't see how you'd fix this issue. Since ts-reset uses a similar approach, it also breaks this sort of call: total-typescript/ts-reset#168

Approach 2: Make BooleanConstructor a type predicate

We can change the call signature of BooleanConstructor to be a type predicate for object types:

diff --git a/src/lib/es5.d.ts b/src/lib/es5.d.ts
index e404df509a..f8e8df6522 100644
--- a/src/lib/es5.d.ts
+++ b/src/lib/es5.d.ts
@@ -532,7 +532,8 @@ interface Boolean {

 interface BooleanConstructor {
     new (value?: any): Boolean;
-    <T>(value?: T): boolean;
+    <T>(value?: T): value is ([T] extends [object | null | undefined] ? T & {} : T);
     readonly prototype: Boolean;
 }

This is conceptually simpler but it also runs into a few problems.

First, TS actually made this change back in 2019 with #29955. But it was quickly reverted in #31515 because it had an undesirable effect on any[].filter(Boolean).

Second, it doesn't work! Even with this change, TS doesn't select the right overload of Array.prototype.filter. You can see this independently of the built-in BooleanConstructor:

declare let dates: (Date|null)[];

interface BCWithNew {
  // Comment out the next line to get "nonNullDates: Date[]" below
  new (value?: any): Boolean;
  <T>(value?: T): value is T & {};
  readonly prototype: Boolean;
}
declare let bcN: BCWithNew;

const nonNullDates = dates.filter(bcN);
//    ^? const nonNullDates: (Date | null)[]

type T = BCWithNew extends (value: Date | null, index: number, array: (Date|null)[]) => value is Date ? true : false;
//   ^? type T = true

(playground)

I think this is a TS bug. @Andarist dug into why this happens, see this thread. My understanding is that it is a correctness / expedience tradeoff in finding the right overload.

The issue around any[].filter(Boolean) returning unknown[] is fixed by #56373. I'm not sure what it would take make TypeScript pick the correct overload of Array.filter.

So in conclusion, it's harder to improve filter(Boolean) than I expected. You may as well use filter(x => !!x) instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

8 participants