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

Fixes to isArray, handled when input is any #56

Closed
wants to merge 4 commits into from
Closed

Fixes to isArray, handled when input is any #56

wants to merge 4 commits into from

Conversation

DeepDoge
Copy link

Handled the cases when the input is any[] or any and not unknown or unknown[]
#48 (comment)

You might wanna check if the test are correct.

@@ -1,3 +1,12 @@
type AsArray<T> = T[] extends T ? T[] : any[]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you inline this? Otherwise it'll be entered into the global scope.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

@DeepDoge DeepDoge requested a review from mattpocock February 22, 2023 18:17
@DeepDoge
Copy link
Author

DeepDoge commented Feb 23, 2023

@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[]

@mattpocock
Copy link
Owner

@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?

@DeepDoge
Copy link
Author

DeepDoge commented Feb 23, 2023

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.

@mattpocock
Copy link
Owner

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.

@DeepDoge
Copy link
Author

DeepDoge commented Feb 23, 2023

@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.

@Gabrielfusf
Copy link

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

@mjxl
Copy link

mjxl commented Mar 20, 2023

@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

@DeepDoge
Copy link
Author

@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.

@ahrjarrett
Copy link

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

@Mickemouse0
Copy link

@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.

@ahrjarrett
Copy link

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!

@DeepDoge
Copy link
Author

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); 
     } 
   } 
 });

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

Successfully merging this pull request may close these issues.

6 participants