feathericons / react-feather

React component for Feather icons
https://npm.im/react-feather
MIT License
1.92k stars 126 forks source link

TypeScript type definitions #13

Closed sgoll closed 6 years ago

sgoll commented 6 years ago

I love this library. Would it be possible to include TypeScript type definitions? This should be as simple as including a file named index.d.ts in the package and (optionally) adding the corresponding "types": "index.d.ts" line to the package.json.

I am attaching a rough and essentially simple draft of the type definitions for the library as of today: https://gist.github.com/sgoll/bf270de030d339215f4964dbd50efc28

brandonmp commented 6 years ago

@sgoll thanks for making these. quick question: why do you pass SVGElement to the generic SVGAttributes?

ie

interface Props extends SVGAttributes<SVGElement> {
  color?: string;
  size?: number;
}

I'm still learning TS and not super clear on generics in general, but i had typed it simply as React.SVGAttributes<T>, and it seemed to work equally well, so just wondering what SVGElement adds to the typing.

mrcrazylee commented 6 years ago

Are there any plans to add these type definitions?! I also really love this icon set, but it would even be more awesome with typing 😄

sgoll commented 6 years ago

@brandonmp SVGAttributes is defined in @types/react/index.d.ts as follows:

interface SVGAttributes<T> extends DOMAttributes<T> { ... }

DOMAttributes in turn uses T in all its event handler attributes to make sure that the events' target property has the correct type: that of the original element, in this case SVGElement.

Since react-feather always creates an SVG element for each icon, this should be typed statically, not itself as generic (because the user is not able to replace the <svg/> with anything else).

All in all, this becomes only relevant when one wants to attach custom event handlers to an icon, such as:

<Activity onClick={...} />

carmelopullara commented 6 years ago

Since I'm not familiar with TypeScript, if someone wants to update the build script to include type definitions, PRs are welcome!

brandonmp commented 6 years ago

@sgoll ahhh that makes sense. thanks for explaining it

@carmelopullara I don't think it requires modding the build script--it should just require including an index.d.ts file in the project root (or maybe in dist?), but I will defer to someone more well-versed in TS than i am.