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

While checking type of something unknown, expect the correct type as input. #50

Closed
DeepDoge opened this issue Feb 22, 2023 · 5 comments
Closed

Comments

@DeepDoge
Copy link

As an example, when we are doing type check on runtime with Array.isArray(args: any)
args is any, first of all it can be unknown which is nicer.

But why not expect an array in the first place? So:

isArray<T extends unknown[] | readonly unknown[]>(arg: T | unknown): arg is T

So when we use it, we get this results:

const arr = [1, 2] as const // readonly [1, 2]
const arr1 = [1, 2] // number[]
const foo: unknown = null // unknown

if (Array.isArray(arr))
  arr // readonly [1, 2]

if (Array.isArray(arr1))
  arr1 // number[]

if (Array.isArray(foo))
  foo // unknown[] | readonly unknown[]

So if we are checking something that is already a known array, we don't make it unknown[]
While we are also still accepting anything unknown.

Also with this, if we know something is not an array or unknown or any we get type error.
Why is that important?
So let's say we had some unknown value in a large function and we check if it's an array.
Then a while later we narrow the type of the value higher in the function, so now we know it's not an array.
For example we check if it's a string and if it's a string, then we don't need to check if a string is an array, because that will always return false.
But now we forget the Array.isArray() check at the bottom, because TS didn't give any type errors.

This should be same for every type check, not just Array.isArray()

Before making a PR and etc. I wanted to here opinions on this.
And if there is a good reason that this is not being done.

@DeepDoge DeepDoge changed the title While checking type of something unknown expect the correct type as input. While checking type of something unknown, expect the correct type as input. Feb 22, 2023
@DeepDoge
Copy link
Author

DeepDoge commented Feb 22, 2023

This also narrows unions.
And fixes this: #48

function bar(): number[] | null {
   return null
}
const barValue = bar() // number[] | null
if (Array.isArray(barValue)) 
  barValue // number[]

@mattpocock
Copy link
Owner

I think the current implementation already covers all the cases you describe. The type annotation you propose would be functionally identical to the one we already have, because T | unknown is the same as unknown.

If you find a failing case with the current implementation, we can re-open.

@mattpocock
Copy link
Owner

Re-opening, just checked #48. Taking a look at this now!

@mattpocock mattpocock reopened this Feb 22, 2023
@DeepDoge
Copy link
Author

DeepDoge commented Feb 22, 2023

@mattpocock Current implementation always returns unknown[], ignores unions and readonly.
Also with this if something already narrowed that we know that it's not an array, this will not let you check if it's an array because that would always return false.

EDIT: ah ok, i saw the problem

@mattpocock
Copy link
Owner

Let's discuss this on #48 itself, because your proposed implementation doesn't fix that issue (checked locally)

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

No branches or pull requests

2 participants