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

Add type guards #43

Merged
merged 4 commits into from
Apr 3, 2018
Merged

Add type guards #43

merged 4 commits into from
Apr 3, 2018

Conversation

SamVerschueren
Copy link
Contributor

I don't have much time now, will have another look tomorrow if we can do some more simplifications with type guards.

@SamVerschueren
Copy link
Contributor Author

Added some more typeguards. The nice thing is that TypeScript now uses the correct type when using is inside an if-else statement for example.

if (is.nativePromise(x)) {
   // `x` is now typed as Promise
}

I believe I can do some more improvements, so PR is WIP.

source/index.ts Outdated
export const object = (value: any) => !nullOrUndefined(value) && (function_(value) || isObject(value));
export const iterable = (value: any) => !nullOrUndefined(value) && function_(value[Symbol.iterator]);
export const generator = (value: any) => iterable(value) && function_(value.next) && function_(value.throw);
export const iterable = <T = any>(value: any): value is Iterator<T> => !nullOrUndefined(value) && function_(value[Symbol.iterator]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows you to write the following

if(is.iterable<string>(foo)) {
   const result = foo.next();
   // `result` is now an `IteratorResult` of type `string`
}

If you want I can remove the generic though as it's not really part of the check. It actually just infers the type of the items inside the Iterator.

source/index.ts Outdated

export const nativePromise = isObjectOfType(TypeName.Promise);
export const nativePromise = isObjectOfType<Promise<any>>(TypeName.Promise);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out how I could make this one generic, like the is.promise check below. Might be weird to be able to pass in a type to .promise but not to .nativePromise.

@@ -7,6 +7,11 @@
"declaration": true,
"pretty": true,
"newLine": "lf",
"lib": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to set this to be able to use SharedArrayBuffer. Should we use es2015 instead because that's our main target?

Copy link
Owner

Choose a reason for hiding this comment

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

Should we use es2015 instead because that's our main target?

Yes

@SamVerschueren
Copy link
Contributor Author

Added some inline comments. I'm not sure about the generic stuff though. Feel free to provide feedback!

@sindresorhus
Copy link
Owner

Added some more typeguards. The nice thing is that TypeScript now uses the correct type when using is inside an if-else statement for example.

Could you document this? Seems like a nice feature users might not be aware of.

@@ -1,5 +1,12 @@
import * as util from 'util';

type TypedArray = Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array;
Copy link

@debuggerpk debuggerpk Apr 1, 2018

Choose a reason for hiding this comment

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

if we do it like

type TypedArray = Array<Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array>;

then typescript will not raise 'cannot find signature on union types' for calling normal functions like reduce, map, indexOf etc.

Choose a reason for hiding this comment

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

if you want, i can add a commit to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although it fixes the issue, the type is incorrect. Now TypedArray is defined as an array of Int8Array or Uint8Array, etc.

What does work however, is the following

type TypedArray = (Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array) & Array<number>;

This is ugly though. Will have a look as to why this error is caused.

Choose a reason for hiding this comment

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

AFAIK, the error message "Cannot invoke an expression whose type lacks a call signature"" is by design. see microsoft/TypeScript#7294 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going to leave it as is for now. It's explained in the issue that you referenced, this is by design. If you know what type it will be, it would be safer to use is.int8Array or one of the others. Thanks for letting me know, learned something new today :).

@SamVerschueren
Copy link
Contributor Author

I thought more about the generic functions we expose, e.g. the following

if (is.promise<number>(foo)) {
   const value = await foo;
   // value is of type number
}

I don't think we should do that. The only thing that is does, is check the type and infer the type correctly. It's not up to is to do more than that. If users type there values correctly, all of it will go automatically.

const foo: Promise<number> | undefined = someValue;

if (is.promise(foo)) {
   const value = await foo;
   // value is of type number
}

If they didn't specify the types correctly, they can always type it themselves.

// foo is of type any

if (is.promise(foo)) {
   const value: number = await foo;
   // value is of type number
}

So I believe we should get rid of the generics to the outside world because it's not up to is to enforce the type the wrapper resolves to. We currently only have generics exposed for is.promise and is.iterable.

@debuggerpk
Copy link

@SamVerschueren thank you for kind reply. It makes sense. I saw the call to review by @sindresorhus on twitter, and looked at it. this particular gave me quite a headache a few weeks ago while upgrading a projecting from js to ts. thank you again.

@SamVerschueren SamVerschueren force-pushed the type-guards branch 3 times, most recently from 163a1d7 to 96ca271 Compare April 3, 2018 05:52
@@ -27,6 +27,32 @@ is.number(6);
//=> true
```

When using `is` together with TypeScript, [type guards](http://www.typescriptlang.org/docs/handbook/advanced-types.html#type-guards-and-differentiating-types) are being used to infer the correct type inside if-else statements.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In VSCode, type inference also works in JavaScript. Don't think we should document this as it's IDE specific.

@SamVerschueren
Copy link
Contributor Author

@sindresorhus Processed feedback. Let me know if you have some more feedback.

@sindresorhus
Copy link
Owner

I don't think we should do that. The only thing that is does, is check the type and infer the type correctly. It's not up to is to do more than that. If users type there values correctly, all of it will go automatically.

Late answer, but yes, I agree.

@sindresorhus sindresorhus merged commit 8894dbe into master Apr 3, 2018
@sindresorhus sindresorhus deleted the type-guards branch April 3, 2018 17:06
@sindresorhus
Copy link
Owner

Yay! Such strong typing 💪 Quality work as always, Sam.

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.

4 participants