kripod / react-polymorphic-types

Zero-runtime polymorphic component definitions for React
MIT License
195 stars 8 forks source link

events have implicit any #8

Closed tu4mo closed 3 years ago

tu4mo commented 3 years ago

Any ideas how to make event-attribute's types work?

https://codesandbox.io/s/onclick-event-rr3rq?file=/src/App.tsx

Mesoptier commented 3 years ago

Unfortunately it's hard to make this work cleanly (I've spend a lot of time trying). In my experience you have two possible workarounds:

// Either explicitly specify the value for the generic T:
<Button<'a'> as="a" foo="bar" onClick={(event) => console.log(event)} />

// Or explicitly specify the type for `event`:
<Button as="button" foo="bar" onClick={(event: React.MouseEvent<HTMLButtonElement>) => console.log(event)} />

I prefer the first option since it's the least amount of additional typing. It obviously comes with the downside of having to specify the element type twice: once as generic (compile-/type-time) and once as value to as (run-time). Note that the type checker forces the two element-types to be exactly the same, so it should never go wrong.

image

tu4mo commented 3 years ago

I appreciate the suggestions @Mesoptier. But having to define the generic type makes the as-prop pretty much useless.

Is this a current restriction in TypeScript?

Mesoptier commented 3 years ago

It seems to me that it is indeed an issue/restriction with TypeScript itself. I've created an issue on their end: https://github.com/microsoft/TypeScript/issues/44596. Hopefully that will lead to either a fix in TypeScript or perhaps to a solution that does not require specifying the element type twice.

Aside: I believe this is the same issue as #5. The only difference being that this issue refers to event calbacks, whereas the other issue is about the ref callback.

marceloadsj commented 3 years ago

I couldn't test it but: https://www.radix-ui.com/docs/primitives/utilities/polymorphic

Seems radix solved this problem, maybe it's worth to have a look.

I am away from my pc now, but I will give it a try soon and I post here!

tu4mo commented 3 years ago

Seems to be working!

https://codesandbox.io/s/peaceful-fire-9nmpt?file=/src/App.tsx

Mesoptier commented 3 years ago

This is great, finally a solution!

@kripod, I believe this is the key part of their solution that react-polymorphic-types doesn't do:

   * We explicitly avoid `React.ElementType` and manually narrow the prop types
   * so that events are typed when using JSX.IntrinsicElements.

https://github.com/radix-ui/primitives/blob/1afa7012fedf8d119b6f07ef86729cacf1152ced/packages/react/polymorphic/src/polymorphic.ts#L41-L42

kripod commented 3 years ago

@Mesoptier thanks for your insightful comment! I figured out how to resolve this and just released v2 with a fix 😊

I think #5 could be fixed in a similar manner.

Mesoptier commented 2 years ago

This issue has now been fixed on TypeScript's end (https://github.com/microsoft/TypeScript/issues/44596), so perhaps this issue can be fixed in a nicer way now?