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 TypeScript support #28

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Stephen-Murphy
Copy link

No breaking changes - adds typescript types for the module unobtrusively.
Fixes #27

Copy link
Contributor

@tunnckoCore tunnckoCore left a comment

Choose a reason for hiding this comment

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

Good one.

@tunnckoCore
Copy link
Contributor

👍

@jonschlinkert
Copy link
Owner

Wow, really sorry that I never noticed this PR. Sometimes I get similar issues on multiple repositories and lose track of which ones I commented or followed up on. I need to be better at that.

I think this is still relevant, however, I'd love to have someone with better TypeScript knowledge than myself review before I merge. Any feedback appreciated.

Copy link
Contributor

@tunnckoCore tunnckoCore left a comment

Choose a reason for hiding this comment

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

Seems good to me.

@tunnckoCore
Copy link
Contributor

tunnckoCore commented Jun 19, 2019

Also, there is a cool testing thing for types tsd by @SamVerschueren.

I'll look on tsd in coming weeks anyway, because I think to use it.

@Stephen-Murphy
Copy link
Author

I'd love to have someone with better TypeScript knowledge than myself review before I merge.
+1

Latest commit includes fixes required by changes to the latest TypeScript, as well as some tweaks I've added from using the this fork myself over the last few months.

With regard to the tests - simply compiling the test/typescript/index.ts file with tsc (I've included npm test-ts script) should be sufficient unless a generated report is needed - AFAIK a tool like tsd wouldn't be of any additional use in our case.

There are a few caveats/edge cases with the types (as noted in the tests/typescript/index.ts file) which I've included here for posterity:

Caveats and Edge Cases

Iterators

TypeScript determines the iterator type by the values returned in the iterator as opposed to the target object being iterated over, whereas kind-of returns the iterator target type as well
The (TypeScript) return type of kindOf((new Map)[Symbol.iterator]()) (and any other iterator regardless of target) will be "mapiterator" | "setiterator" | "stringiterator" | "arrayiterator".
To test for an iterator regardless of source you would simply use kindOf(foo).endsWith("iterator").

Functions/GeneratorFunctions

Generator functions return IterableIterator<T>, but a 'normal' function could as well, so the TypeScript return type of kindOf(*generator...) is "generatorfunction" | "function" as opposed to just "generatorfunction" as TypeScript has no way to distinguish the two (to the best of my knowledge).
To test for a function regardless of type you would simply use kindOf(foo).endsWith("function").
'Normal' functions that return any other type will be correctly typed as "function" i.e. kindOf(() => null) == "function".

Error-like, RegExp-like, Generator-like, Date-like

These type checks will reflect the fact that kind-of has fallback checks for RegExp, Error, Generator, and Date objects to check for "RegExp-like" and "Error-like" etc...

  • The return type of kindOf({ flags: "", ignoreCase: true, multiline: true, global: true }) will be "regexp".
  • The return type of kindOf({ message: "", constructor: { stackTraceLimit: 0 } }) will be "error".
  • The return type of kindOf({ throw: () => {}, return: () => {}, next: () => {} }) will be "generator".
  • The return type of kindOf({ toDateString: () => {}, getDate: () => {}, setDate: () => {} }) will be "date".

Buffers

Buffers in environments where Buffer.isBuffer (or an appropriate polyfill) is unavailable may not correctly be typed as "buffer".
e.g. kindOf(Buffer.from([])) could return "uint8array" but TypeScript may think the return type is "buffer".

@simonwep
Copy link

simonwep commented May 5, 2020

Is this going to get merged at some time?

@colshacol
Copy link

LGTM!

(Sorry to bump without actually having substance to add...)

@jimmywarting
Copy link

I would prefer if u stick to js and used jsdoc instead
https://www.youtube.com/watch?v=iP5XwRT2tNw

@alii
Copy link

alii commented Sep 30, 2021

LGTM

@mindplay-dk
Copy link

I hope you don't mind my asking, but what is this for?

I mean, what's the use of getting a specific, well-known const string type like "symbol" or "buffer"?

Because it seems to me, in Typescript, if the type is actually known already, getting a less specific const string representation for the "type of the type" is probably not relevant.

It seems the more likely use case is you don't know the type - I mean, why else would you be calling this function to check the type, right? And if you don't know the type, the compile-time type will resolve to string anyhow, so...

I understand why this is "technically correct", but when or how would you use this?

When is it more useful than just getting string, or possibly just "symbol" | "buffer" | ... ?

@alii
Copy link

alii commented Mar 30, 2022

TypeScript can take us a lot further with control flow with having known literal types. For example

declare const x: "yes" | "no";

if (x === "yes") {
	// x must be `yes` here, nothing else
	x;
}

Now this looks fairly useless in this example, but say we had a more complex object where the only discriminatory type is the string literal. Let's take a look at what that might look like and why it might be useful:

// Some random objects that share a `.type` property
declare const options = {type: "boolean", state: true} | {type: "number", asInteger: 1};

if (options.type === "boolean") {
	// Safely access `.state` here
	options.state;
} else {
	// Safely access `.asInteger` here.
	options.asInteger;
}

This is only possible because we have literal values known for the strings. There is also a lot more you can do (like pulling URL params out of /users/:id), but that is out of scope for my comment 😄 .

@mindplay-dk
Copy link

@alii I think you misunderstood me - I've used constant string types, narrowing, and discriminators. I wasn't looking for a general explanation of the language feature. I'm wondering when or how it's useful, specifically, in the context of this library?

@alii
Copy link

alii commented Mar 30, 2022

Ahh please accept my apology for the misunderstanding. I can't seem to really think of a direct use case in this context, but at the very least it's nice to have the overloads for kindOf even if the usefulness of the strict return types are not immediately obvious useful. 😄

@mindplay-dk
Copy link

Ahh please accept my apology for the misunderstanding. I can't seem to really think of a direct use case in this context, but at the very least it's nice to have the overloads for kindOf even if the usefulness of the strict return types are not immediately obvious useful. 😄

My question is, are they useful at all? To my previous point:

It seems the more likely use case is you don't know the type - I mean, why else would you be calling this function to check the type, right? And if you don't know the type, the compile-time type will resolve to string anyhow, so...

I'm not trying to say it's harmful - but having this level of "typeness" might be confusing more than helpful, unless someone can think of an actual use for it.

Does the author of the PR know what they're for? 🙂

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.

Can support typescript?
9 participants