mantinedev / mantine

A fully featured React components library
https://mantine.dev
MIT License
26.87k stars 1.9k forks source link

Typescript incremental recompile is a little slow #812

Closed robeady closed 2 years ago

robeady commented 2 years ago

What package has an issue

@mantine/core

Describe the bug

I added mantine to an almost empty project replacing chakra-ui, and I noticed immediately that autocomplete and incremental compilation checks became frustratingly slower.

I ran tsc --watch --diagnostics and observed that check time for any code change was now 1-2 seconds, compared to around 0.4 seconds previously, including for trivial changes such as modifying a className string.

Digging a bit deeper, I found that this problem did not occur when using some components e.g. Modal. But when I added a Button or a Box it did occur.

I started messing around with the types and noticed that if I replaced for example the generic type of ButtonComponent

declare type ButtonComponent = (<C extends React.ElementType = 'button'>(props: ButtonProps<C>) => React.ReactElement) 

with a non generic

declare type ButtonComponent = ((props: ButtonProps<"button">) => React.ReactElement)

Then incremental compilation suddenly became faster for trivial changes such as modifying a className string, i.e. around 0.1 seconds check time. Same for the Box component.

I haven't used the library enough to understand the importance of these generic types, but I wonder if there is a trade-off here.

The observation didn't fully surprise me, because I have seen in typings for other libraries e.g. styled-components that very large string unions, as found in React.ElementType, can make type checking slow. Maybe the generic types have the side effect that the compiler has to constantly re-evaluate such types when it otherwise wouldn't during incremental recompilation.

In which browser did the problem occur

-

If possible, please include a link to a codesandbox with the reproduced problem

No response

Do you know how to fix the issue

No

Are you willing to participate in fixing this issue and create a pull request with the fix

Yes

Possible fix

No response

robeady commented 2 years ago

Hmm, I may have a potential fix. I haven't tested this outside of simple scenarios, but if I push the generic bound down into a conditional type instead, the slowness for trivial changes seems to be cured...

- export declare type ButtonProps<C extends React.ElementType> = PolymorphicComponentProps<C, SharedButtonProps>;
+ export declare type ButtonProps<C> = C extends React.ElementType ? PolymorphicComponentProps<C, SharedButtonProps> : never;
- declare type ButtonComponent = (<C extends React.ElementType = 'button'>(props: ButtonProps<C>) => React.ReactElement) & {
+ declare type ButtonComponent = (<C = 'button'>(props: ButtonProps<C>) => React.ReactElement) & {

This approach still verifies that the generic type is valid, although it does make the compile errors for such a scenario a little confusing... if I write <Button<"wrong"> ... /> I get compile errors everywhere except on the generic type argument (because props are now typed as never)! But I suspect that's acceptable and/or there is probably some trick I don't know about to get the compiler to print a more useful error message.

Edit: well if error messages are important, the following approach seems to kind of work:

export declare type ButtonProps<C> = C extends React.ElementType
    ? PolymorphicComponentProps<C, SharedButtonProps>
    : "Error: generic parameter is not a valid React element type";

User code:

<Button<"wrong"> className="foo" onClick={() => {}} />

Error (squiggle under "Button"):

Type '{ children: string; className: string; onClick: () => void; }' is not assignable to type '"Error: generic parameter is not a valid React element type"'

This seems safe to me because JSX won't let you specify props as a literal string type but I'm not an expert.

rtivital commented 2 years ago

Thanks for exploring the issue, we've discussed the issue with slow TypeScript several times on Discord but were not able to find a solution that will not be a breaking change. I'll try your approach and will release alpha version soon to try things out.

rtivital commented 2 years ago

We've discussed some details on Discord and if anyone else is interested – the changes described in this issue were published in 3.7.0-alpha.0 release and can be tested by anyone.

rtivital commented 2 years ago

I've released 3.6.7 release with changes that we've discussed on Discord, thanks again for proposing a good solution!