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

[@mantine/core] clearer typescript error for polymorphic components #924

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

jvdsande
Copy link
Contributor

@jvdsande jvdsande commented Mar 3, 2022

As discussed on Discord, this PR changes the following structure:

C extends React.ElementType ? PolymorphicComponentProps<C, SomeProps>  : never

to

C extends React.ElementType ? PolymorphicComponentProps<C, SomeProps>  : (SomeProps & { component: React.ElementType })

This allows for clearer error messages when providing a wrong value as component: instead of having all props failing, now only the component prop fails.

Furthermore it moves the conditional typing logic inside PolymorphicComponentProps directly to avoid duplicating it everywhere.

Note: one use case was not discussed on Discord, it's using the "generic" structure:

<Avatar<wrong> size="sm" />

Then the error is a bit less straightfoward, because it starts by saying that component prop is missing while required, then when we change to:

<Avatar<wrong> size="sm" component="wrong" />

The error moves to "component" and says "Type '"wrong"' is not assignable to type 'ElementType'"

I'm not quite sure why the docs tells users to use this structure though since simply providing the component props makes TS infer correctly the other props too. Using the generic component instance structure is redundant in this situation.

@rtivital rtivital merged commit 0d76555 into mantinedev:master Mar 3, 2022
@rtivital
Copy link
Member

rtivital commented Mar 3, 2022

Thanks for PR!

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.

2 participants