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

.filter() can't handle union of arrays #168

Open
mattiaz9 opened this issue Sep 8, 2023 · 9 comments
Open

.filter() can't handle union of arrays #168

mattiaz9 opened this issue Sep 8, 2023 · 9 comments

Comments

@mattiaz9
Copy link

mattiaz9 commented Sep 8, 2023

Example:

type ClientA = { firstName: string; lastName: string; isAdmin: boolean }
type ClientB = { fullName: string; isAdmin: boolean }
type ClientList = ClientA[] | ClientB[]

const list: ClientList = []
const admins = list.filter(c => c.isAdmin)
//                         ^^^^^^^^^^^^^^
// Error: Argument of type '<T>(c: T | undefined) => any' is not assignable to parameter of type 'BooleanConstructor'. 
// Type '<T>(c: T | undefined) => any' provides no match for the signature 'new (value?: any): Boolean'

In this example both ClientA and ClientB has a common field isAdmin but .filter() can't handle this union.

Removing ts-reset fixed the error.
Also changing the type to (ClientA | ClientB)[] fixed the error, but it can be inconvenient in some cases because of a forced type cast.

@cobaltt7
Copy link

cobaltt7 commented Nov 8, 2023

Looks like using a generic fixes this error:

filter<P extends (value: T, index: number, array: T[]) => unknown>(
	predicate: P,
	thisArg?: unknown,
): P extends BooleanConstructor ? NonFalsy<T>[] : T[];

image
image

@mattpocock
Copy link
Owner

This was fixed upstream in TS I think!

@m-radzikowski
Copy link

Unfortunately, this still occurs with the latest TypeScript 5.3.3.

@sillvva
Copy link

sillvva commented Mar 17, 2024

Yeah, this is weird.

CleanShot 2024-03-17 at 09 54 10@2x
CleanShot 2024-03-17 at 09 53 18@2x

@sillvva
Copy link

sillvva commented Mar 20, 2024

@mattpocock you forgot to add import "@total-typescript/ts-reset";. Doing so breaks that code.

https://www.typescriptlang.org/play?ssl=1&ssc=9&pln=1&pc=35#code/JYWwDg9gTgLgBAIgAIwjAhgGwLQwJ5gCmAzgMZTBgwD0Mx2UJhMCA3AFDv5FwDCmwQgDsYAQTgBeOAG84AM2BRiMAHLoQhAFxxlFIQHNWcTOmVqN23cANHgxUQBMQ17QCMIETIXRC4AXy4CQj4BYRgAIUkZeQBXTExzLR0YPUM4O0dnITcPLx9-QJ5+QREAGTt4KWKw0QBtAF04AB8QkoiGzlIIIWVjCu1qsoqojq6e+HQna2IogWUAOgVMGEIoAApSSQA+OFJ5jKmhAEogA

CleanShot 2024-03-20 at 14 53 40@2x


Update:

Looking at the code for ts-reset, this fixes it without breaking .filter(Boolean)

/// <reference path="utils.d.ts" />

interface Array<T> {
+ filter(predicate: (item: T, index: number, array: T[]) => boolean, thisArg?: any): TSReset.NonFalsy<T>[];
- filter(predicate: BooleanConstructor, thisArg?: any): TSReset.NonFalsy<T>[];
}

interface ReadonlyArray<T> {
+ filter(predicate: (item: T, index: number, array: T[]) => boolean, thisArg?: any): TSReset.NonFalsy<T>[];
- filter(predicate: BooleanConstructor, thisArg?: any): TSReset.NonFalsy<T>[];
}

Here is an updated example:

https://www.typescriptlang.org/play?#code/C4TwDgpgBAYghgGwM4igXigM0U6AfKABigICJSSoA7AVwQUpqoBMJMBLKiZqAKFEhQAcgHsq8ZCAA8AFQB86KAFEAHgGMENVrIA0sHCDkBuXr07AIAJ2xroAQUuW40+VADevAJAcEFywAowS252NTgLAC4of3YLAFsomQBKdAUAIxERBAg4Kj1gAAt2JAcAcwB+KNyQJKjRcQNZOQBtAF0TAF9TAWgAYQR2CCpgO0U3LHZLJGAhODiIKOnLTlKjKAQ4adn5xeBlqlWoYrtmOM4ojKycqigunqh+weGAITGsOgRthagllbXj07nKCXbK5W78cB9AZDYAAGWKwEU-keMNGBCYrA4XGYSTalBRLzapjUYmm6wRUQJcIRiiJJKoZLggIZigG0wAdD4-P5nplQVQkpz2L4rP41KkoGp2QCzgKgA

@mattpocock mattpocock reopened this Mar 21, 2024
@sillvva
Copy link

sillvva commented Mar 21, 2024

Hmm. Looks like the above solution breaks type predicate filters.

// With the interface
const list1 = ["0", 0, false];
const list2 = list1.filter((v): v is number => typeof v === "number");
//    ^? (string | number | true)[]
console.log(list2); // 0

// Without the interface
const list1 = ["0", 0, false];
const list2 = list1.filter((v): v is number => typeof v === "number");
//    ^? number[]
console.log(list2); // 0

@danvk
Copy link

danvk commented Apr 4, 2024

This is actually a pretty fundamental problem with the ts-reset approach that I think will be hard to fix. microsoft/TypeScript#53489 added special logic for calling methods on A[] | B[]. If there are no overloads that work on A[] | B[] then it tries (A | B)[] instead. When you add a .filter(Boolean) overload, there's always one valid overload and the fallback never triggers.

I think adding an overload to BooleanConstructor itself might be a more promising approach (microsoft/TypeScript#16655) but it also has some issues, see this thread.

@cobaltt7
Copy link

cobaltt7 commented Apr 4, 2024

interface Array<T> {
	filter<S extends T>(
		predicate: (value: T, index: number, array: readonly T[]) => value is S,
		thisArg?: unknown,
	): S[]; // duplicated from DOM types
	filter<P extends (value: T, index: number, array: T[]) => unknown>(
		predicate: P,
		thisArg?: unknown,
	): (P extends BooleanConstructor ? NonFalsy<T> : T)[];
}

I've been using this solution without problems for a while, although I haven't looked much into why it works. Interstingly, the overloads have to be in this order for it to work. The BooleanConstructor overload must override the predefined DOM overload and duplicating the DOM overload somehow prevents that.
Playground Link

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

No branches or pull requests

6 participants