mattpocock / ts-reset

A 'CSS reset' for TypeScript, improving types for common JavaScript API's
https://www.totaltypescript.com/ts-reset
MIT License
7.9k stars 124 forks source link

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

Open mattiaz9 opened 1 year ago

mattiaz9 commented 1 year ago

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 commented 12 months ago

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 commented 12 months ago

This was fixed upstream in TS I think!

m-radzikowski commented 9 months ago

Unfortunately, this still occurs with the latest TypeScript 5.3.3.

sillvva commented 7 months ago

Yeah, this is weird.

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

mattpocock commented 7 months ago

Looks fixed in this playground:

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

sillvva commented 7 months ago

@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

sillvva commented 7 months ago

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 commented 7 months ago

This is actually a pretty fundamental problem with the ts-reset approach that I think will be hard to fix. https://github.com/microsoft/TypeScript/pull/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 (https://github.com/microsoft/TypeScript/issues/16655) but it also has some issues, see this thread.

cobaltt7 commented 7 months ago
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