-
Notifications
You must be signed in to change notification settings - Fork 118
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
Improve type narrowing for .includes
#49
Comments
Oh, this is good! const test = [1, 2, 3] as const;
const test2 = [...test, 4] as const;
const test3 = [0, 2, 3] as const;
let val = 0;
if (test2.includes(val)) {
console.log(val) // 1 | 2 | 3 | 4
if (!(test.includes(val))) {
console.log(val) // 4
}
}
if (test.includes(val)) {
console.log(val) // 1 | 2 | 3
if (!(test2.includes(val))) {
console.log(val) // never
}
}
if (test3.includes(val)) {
console.log(val) // 0 | 2 | 3
if (!(test2.includes(val))) {
return val // 0
}
console.log(val) // 2 | 3
} |
This seems like a good idea! I don't see why this couldn't be included. I'm going to let this sit for a while to percolate to see if I can think of any issues with this. |
I could also do a PR and update the tests. That might surface a few issues. Should be pretty easy. |
@essenmitsosse Do it! |
I am a bit confused. I just added the tests that were supposed to fail, but it is actually already working, even without any changes to the implementation. It looks like TypeScript is creating the union by itself in those cases — there is no need for the type predicate to narrow. I wonder why it didn't seem to work when I first tried it. Another thing that might be answered somewhere, but I couldn't find it: Why doesn't |
@essenmitsosse It got raised on a Twitter thread somewhere. Basically, on an array with an unknown number of members you can't guarantee that a member of the union is or isn't in the array. So you end up with some WEIRD situations where it gets inferred as 'never'. |
For someone stumbling over this in the future: I tracked down the tweet you were talking about function dedupe<T>(items: readonly T[]): T[] {
const newArray: T[] = []
for (const item of items) {
if (newArray.includes(item)) continue
newArray.push(item)
// ^? const item: never
}
return newArray
} This is with the typing of |
@mattpocock What I am wondering about above code: If that is a bug, why isn't this one a bug as well? function dedupe2<T>(items: readonly T[]): readonly T[] {
const newArray: readonly T[] = [1 as T]
for (const item of items) {
if (!newArray.includes(item)) {
console.log(item)
// ^? const item: never
}
}
return newArray
} While I see the problem, it seems unrelated to the Array being writable. |
A simpler example: const validNumbers: readonly number[] = [1, 2, 3]
const checkNumber = (items: readonly number[]): readonly string[] =>
items.map(item => validNumbers.includes(item) ? `${item}` : `!${item}`)
// ^? const item: never |
imo both |
That was my thought as well, but the current implementation makes a difference between |
Here is an even worse example: const validNum: ReadonlyArray<number> = [1, 5]
const example234 = [2, 3, 4] as const
example234.map((item) => validNum.includes(item)
? `${item}`
// ^? const item: 2, 3, 4
: `!${item}`)
// ^? const item: never In this case, even the initial type narrowing is wrong, not just the inverse. |
My heuristic was that if you're creating a It's extremely rare that you're creating BUT I am willing to be talked around if we can figure out a practical example. |
This might be a coding style issue. For me, almost all Arrays are |
Or to put it another way: if you happen to have a |
interface Order {
listIdProduct: ReadonlyArray<number>,
}
const removeId = (order: Order, id: number) => {
if (order.listIdProduct.includes(id) ) {
return order.listIdProduct.filter(/* yadayadayada */);
}
return `ID ${id.toString()} not included in order`;
// Property 'toString' does not exist on type 'never'
} This example is a bit contrived, but the general idea here shouldn't be that uncommon. All you need to do is get your read-only Array from an object where it isn't supposed to be mutated, but you don't know what the specific members are. Or just write all functions to take read-only arrays as arguments so you don't mutate them. |
As far as I understand it, it is not solvable for general arrays because of how type guards work in TypeScript. If you check a type guard in one branch of your code (e.g. if), the type in the other branch (e.g. else) will be the "opposite". So for a type guard implementation it's not enough to say: If the check is true, the variable has the type T. And that is simply not the case for general arrays, as shown by the many examples in this thread. I think however there is a subclass where it is possible to narrow: If the array is a tuple with well known entries. And that is probably the most common use case where narrowing with Array.includes is useful, so we might be in luck. const possibleValues = ['a', 'b', 'c'] as const // readonly ['a', 'b', 'c']
declare const x: string
if (possibleValues.includes(x3)) {
console.log(x3);
// ^? // 'a' | 'b' | 'c'
} else {
console.log(x3);
// ^? // string
} I have made a pull request #130. In the PR I have described a case where this doesn't work well, which might or might not be a deal breaker. Let me know what you think. |
|
Given the following code:
we get a Typeguard that
SOME_VALUE
is'a' | 'b' | 'c'
— but in this case we know more about: we know it is not 'c' so it would be great to not have 'c' show up in the result again. The typeguard should narrow to'a' | 'b'
One naive way to implement it would be something like:
Diff:
Is there any reason this shouldn't be done? If not I'd be glad to open a PR.
The text was updated successfully, but these errors were encountered: