ts-reset icon indicating copy to clipboard operation
ts-reset copied to clipboard

Fixes to isArray, handled when input is `any`

Open DeepDoge opened this issue 2 years ago • 6 comments

Handled the cases when the input is any[] or any and not unknown or unknown[] https://github.com/total-typescript/ts-reset/issues/48#issuecomment-1440269813

You might wanna check if the test are correct.

DeepDoge avatar Feb 22 '23 15:02 DeepDoge

@mattpocock Found a better solution that returns less complex type for Unarray<T> test.

interface ArrayConstructor {
  // @ts-ignore
  isArray<T>(arg: T[] extends T ? T | T[] : never): arg is unknown[];
}

Basically, this ^ only works when the input is any[], any, unknown or unknown[], for other types, it fallbacks to the default isArray() definition.

I have one question, when the input is any[] or any, should it return unknown[] or any[]? Code above returns unknown[] for any[], any, unknown or unknown[]. If you want this behavior you can merge.

Or I can change it to the example below:

interface ArrayConstructor {
  // @ts-ignore
  isArray<T>(arg: T[] extends T ? T | T[] : never): arg is T[];
}

This ^ would return:

  • unknown[] for unknown or unknown[]
  • any[] for any or any[]

DeepDoge avatar Feb 23 '23 02:02 DeepDoge

@DeepDoge I'm not sure that we can have // @ts-ignore in the bundle - experience from working on XState is that those can often cause unexpected surprises when bundled.

Do you have a test case that doesn't pass with the current implementation?

mattpocock avatar Feb 23 '23 09:02 mattpocock

i mean first solution was passing the tests, down side was.

On this test:

doNotExecute(() => {
  function test<T>(value: T) {
    type Unarray<T> = T extends Array<infer U> ? U : T;
    const inner = <X extends Unarray<T>>(v: X[]) => {};

    if (Array.isArray(value)) {
      inner(value);
    }
  }
});

valuein inner(value); was returning a complex type, which wasn't that nice to look at. Let me see if I can get rid of // @ts-ignore, if I can't, I will rollback to the previous commit.

DeepDoge avatar Feb 23 '23 09:02 DeepDoge

value in inner(value); was returning a complex type, which wasn't that nice to look at.

Gotcha, I don't mind too much because especially inside a generic function, it's fairly important to see how T is altered.

mattpocock avatar Feb 23 '23 09:02 mattpocock

@mattpocock Wasn't expecting to solve it this quick. Got rid of @ts-ignore

Current behavior: Input => Result

  • any => any[]
  • any[] => any[]
  • unknown => unknown[]
  • unknown[] => unknown[]
  • other => fallbacks to the default Array.isArray() definition

Can be merged now.

DeepDoge avatar Feb 23 '23 09:02 DeepDoge

isArray<T>(arg: T[] extends T ? T | T[] : never): arg is unknown[];

Gabrielfusf avatar Feb 23 '23 11:02 Gabrielfusf

@mattpocock @DeepDoge

I was messing around with this, and i came up with this one

interface ArrayConstructor {
  isArray<T>(
    arg: T
  ): T extends Array<unknown> | ReadonlyArray<unknown> ? true : false
}

let array = Array.isArray(123) // false
array = Array.isArray([1, 2, 3]) // true

mjxl avatar Mar 20 '23 13:03 mjxl

@mjxl This PR is for narrowing the type correctly. Not for returning true or false, based on argument type.

But, if you ask me if the type of the argument is already known to be not an array, it shouldn't even let you use it as an argument. But we can't do this because it will just fallback to the default isArray definition of typescript.

DeepDoge avatar Mar 20 '23 15:03 DeepDoge

I'm not sure I totally understand the use case here, but did want to chime in and argue against adding a type parameter to isArray, for the same reasons outlined in the README

ahrjarrett avatar May 11 '23 17:05 ahrjarrett

@DeepDoge Would it be possible to make this handle readonly arrays as well? With the current implementation these tests fail.

doNotExecute(() => {
  const arrOrString = [] as readonly string[] | string;

  if (Array.isArray(arrOrString)) {
    type tests = [Expect<Equal<typeof arrOrString, readonly string[]>>];
  }
});

doNotExecute(() => {
  const arrOrString = [] as readonly string[] | string;

  if (Array.isArray(arrOrString)) return;
  type tests = [Expect<Equal<typeof arrOrString, string>>];
});

I have been testing a bit but can't quite figure it out. With

interface ArrayConstructor {
  isArray<T>(arg: T): arg is T extends readonly any[] | any[] ? T : T[] extends T ? T[] : never;
}

these tests fail

doNotExecute(() => {
  const maybeArr = [1, 2, 3] as any;

  if (Array.isArray(maybeArr)) {
    type tests = [Expect<Equal<typeof maybeArr, any[]>>];
  }
});

doNotExecute(() => {
  function test<T>(value: T) {
    type Unarray<T> = T extends Array<infer U> ? U : T;
    const inner = <X extends Unarray<T>>(v: X[]) => {};

    if (Array.isArray(value)) {
      inner(value);
    }
  }
});

and with

interface ArrayConstructor {
  isArray<T>(arg: T[] extends T ? T | T[] : never): arg is T[] extends T ? T[] : never;
  isArray<T>(arg: T): arg is T extends readonly any[] | any[] ? T : T[] extends T ? T[] : never;
}

only

doNotExecute(() => {
  function test<T>(value: T) {
    type Unarray<T> = T extends Array<infer U> ? U : T;
    const inner = <X extends Unarray<T>>(v: X[]) => {};

    if (Array.isArray(value)) {
      inner(value);
    }
  }
});

fails but I can't figure out how to make all of them pass.

Mickemouse0 avatar Jun 16 '23 17:06 Mickemouse0

Hey @DeepDoge , not sure if this is related or not, but if you're trying to add support for readonly arrays, the easiest way to do that is to treat all arrays like readonly arrays.

That's because ReadonlyArray is the supertype -- which is kinda backwards from what most people expect.

Here's a sandbox about it: https://codesandbox.io/s/arrays-vs-readonlyarray-in-typescript-xdgsfj?file=/src/array-stuff.ts

That's something I wish someone had told me, because I found this behavior very confusing until I figured out why it worked that way.

Hopefully that's helpful!

ahrjarrett avatar Jun 16 '23 22:06 ahrjarrett

Although Matt told me to open this PR, I don't think this is gonna get merged, it has been months. I don't wanna deal with this PR anymore. So closing it and deleting the fork.

For future reference, here's the code:

interface ArrayConstructor { 
   isArray<T>(arg: T[] extends T ? T | T[] : never): arg is T[] extends T ? T[] : never; 
 }

And tests:

import { doNotExecute, Equal, Expect } from "./utils"; 
  
 doNotExecute(() => { 
   const maybeArr = [1, 2, 3] as unknown; 
  
   if (Array.isArray(maybeArr)) { 
     type tests = [Expect<Equal<typeof maybeArr, unknown[]>>]; 
   } 
 }); 
  
 doNotExecute(() => { 
   const maybeArr = [1, 2, 3] as unknown[]; 
  
   if (Array.isArray(maybeArr)) { 
     type tests = [Expect<Equal<typeof maybeArr, unknown[]>>]; 
   } 
 }); 
  
 doNotExecute(() => { 
   const maybeArr = [1, 2, 3] as any; 
  
   if (Array.isArray(maybeArr)) { 
     type tests = [Expect<Equal<typeof maybeArr, any[]>>]; 
   } 
 }); 
  
 doNotExecute(() => { 
   const maybeArr = [1, 2, 3] as any[]; 
  
   if (Array.isArray(maybeArr)) { 
     type tests = [Expect<Equal<typeof maybeArr, any[]>>]; 
   } 
 }); 
  
 doNotExecute(() => { 
   const arrOrString = [] as string[] | string; 
  
   if (Array.isArray(arrOrString)) { 
     type tests = [Expect<Equal<typeof arrOrString, string[]>>]; 
   } 
 }); 
  
 doNotExecute(() => { 
   const arrOrString = [] as string[] | string; 
  
   if (Array.isArray(arrOrString)) return; 
   type tests = [Expect<Equal<typeof arrOrString, string>>]; 
 }); 
  
 doNotExecute(() => { 
   let arrOrString = "" as string | [1, 2]; 
  
   if (Array.isArray(arrOrString)) { 
     type tests = [Expect<Equal<typeof arrOrString, [1, 2]>>]; 
   } 
 }); 
  
 doNotExecute(() => { 
   let path = [] as string | string[]; 
  
   const paths = Array.isArray(path) ? path : [path]; 
  
   type tests = [Expect<Equal<typeof paths, string[]>>]; 
 }); 
  
 doNotExecute(() => { 
   function test<T>(value: T) { 
     type Unarray<T> = T extends Array<infer U> ? U : T; 
     const inner = <X extends Unarray<T>>(v: X[]) => {}; 
  
     if (Array.isArray(value)) { 
       inner(value); 
     } 
   } 
 });

DeepDoge avatar Jun 16 '23 22:06 DeepDoge