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

Improvment to `Array.isArray`

Open Mickemouse0 opened this issue 2 years ago • 7 comments

This PR improves the type inference of Array.isArray.

It builds on the changes made in https://github.com/total-typescript/ts-reset/pull/56 and https://github.com/total-typescript/ts-reset/pull/23 to solve https://github.com/total-typescript/ts-reset/issues/48.

Mickemouse0 avatar Jun 17 '23 23:06 Mickemouse0

@mattpocock @DeepDoge @phenomnomnominal I have not been able to make it pass the final test. If you have any suggestions I'd be happy for the help.

Mickemouse0 avatar Jun 17 '23 23:06 Mickemouse0

I have been able to get either or of

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

doNotExecute(async () => {
  function makeArray<Item>(input: Item | ReadonlyArray<Item> | Array<Item>) {
    if (Array.isArray(input)) {
      return input;
    }
    return [input];
  }

  const [first] = makeArray([{ a: "1" }, { a: "2" }, { a: "3" }] as const);

  // No error!
  first.a;
});

to pass but not both at the same time.

With

interface ArrayConstructor {
  isArray<T>(arg: true extends TSReset.IsAny<T> ? never : T): arg is true extends TSReset.IsAny<T> ? never : T extends unknown[] | readonly unknown[] ? T : T[] extends T ? T[] : never;
}

the first passes and with

interface ArrayConstructor {
  isArray<T>(arg: true extends TSReset.IsAny<T> ? never : T): arg is true extends TSReset.IsAny<T> ? never : T extends unknown[] | readonly unknown[] ? T : T[] extends T ? T[] : never;
  isArray<T>(arg: T[] extends T ? T | T[] : never): arg is T[] extends T ? T[] : never;
  isArray<T>(arg: T): arg is T extends unknown[] | readonly unknown[] ? T : T[] extends T ? T[] : never;
}

the second passes.

Any help to get both to pass would be greatly appreciated.

Mickemouse0 avatar Jun 18 '23 08:06 Mickemouse0

doNotExecute(async () => {
  function makeArray<Item>(input: Item) {
    if (Array.isArray(input)) {
      return input;
    }
    return [input];
  }

  const [first] = makeArray([{ a: "1" }, { a: "2" }, { a: "3" }] as const);

  // No error!
  first.a;
});

In the test above, you get a value, and if that value is an array, you just return it, if its not an array you place it inside an array and return it.

So as far as typescript knows that makeArray function can either return [Item] or Item, so it's return type is Item | [Item]. It doesn't care about your runtime logic. So this is a typescript issue, how it works, can't fix it.

Normally to make it give the right type you would define the makeArray function like this.

function makeArray<Item>(input: Item): 
    Item extends Array<any> ? Item : 
    Item extends ReadonlyArray<any> ? Item : 
    [Item]
  {
     if (Array.isArray(input)) {
       return input as any;
     }
     return [input] as any;
  }

So, basically you have to repeat your runtime logic with types. It's a typescript issue, not an issue with isArray().

As far as I know, but a second opinion won't hurt, maybe I'm missing something. Tbh, I was thinking about making my own scripting or programming language to overcome this issue and more.

DeepDoge avatar Jun 18 '23 09:06 DeepDoge

Thanks @DeepDoge for your answer. If @mattpocock agrees I could just remove that test and it would be ready from my side.

Mickemouse0 avatar Jun 18 '23 09:06 Mickemouse0

I've tried this and it didn't work for my case. However this PR seems to work for my case where I want to narrow type from string or string array:

const token: string | readonly string[];
if (Array.isArray(token)) throw new Error("Token is an array");
console.log(token); // token is `string`, not `string | readonly string[]` here (as expected)

Maxim-Mazurok avatar Jun 23 '23 04:06 Maxim-Mazurok

@mattpocock I removed the failing test based on this comment. This PR is now redo to be merged

Mickemouse0 avatar Jun 23 '23 09:06 Mickemouse0

@mattpocock Are the changes in this PR something that will be considered?

Mickemouse0 avatar May 29 '24 19:05 Mickemouse0