[core] Offer alternative to OverridableComponent via module augmentation for better performance

[mnajdova]

Related to #19113

OverridableComponent

The bottleneck of the OverridableComponent turned out to be the usage of ComponentPropsWithRef, it took most of the time in the type check. Replacing this with ComponentPropsWithoutRef & { ref?: React.Ref<any> } yield in considerable improvement of the type checks length (check the pictures below). I am intentionally not using percentage, as different runs can take up different times, but if you check the length of the checkSourceFile element, it is visible that it takes much less time compared to the other things done by the typescript engine.

The benchmarks use CRA with the following App.tsx

import * as React from 'react';
import CardContent from '@mui/material/CardContent';

export default function App() {
  return (
    <React.Fragment>
      <CardContent>Some default card content</CardContent>
      <CardContent component="a" href='https://google.com'>Card content as 'a'</CardContent>
    </React.Fragment>
  )
}

Usage

import type {} from '@mui/material/OverridableComponentAugmentation';
// and if you use @mui/base
import type {} from '@mui/types/OverridableComponentAugmentation';

Difference

master

App checkSourceFile duration - 1,935.307 ms

PR

App checkSourceFile duration - 273.175 ms

Breaking change

The refs in the ref callbacks prop are going to be Element instead of reflect the component used in the component prop. For e.g.:

index 883ff46b1f..a7b4ba5179 100644
--- a/packages/mui-material/test/typescript/components.spec.tsx
+++ b/packages/mui-material/test/typescript/components.spec.tsx
@@ -136,7 +136,7 @@ const AvatarTest = () => (
     <Avatar<typeof TestOverride>
       component={TestOverride}
       ref={(elem) => {
-        expectType<HTMLDivElement | null, typeof elem>(elem);
+        expectType<Element | null, typeof elem>(elem);
       }}
       x={3}
       alt="Image Alt"

[mui-bot]

Details of bundle changes

@material-ui/core: parsed: +19.61% , gzip: +11.07%
@mui/material-next: parsed: +136.40% , gzip: +121.20%

Generated by 🚫 dangerJS against 97b0ed9

[michaldudak]

Great work and an awesome outcome! I've noted a few things to think about when using this pattern in the whole codebase

[siriwatknp]

Do you think the usage of DistributiveOmit here could be another bottleneck?

The reason I ask is that the React.ComponentPropsWithoutRef has a lot of keys and omitting some of the keys sounds like it requires expensive computation.

[michaldudak]

I don't think we can get away without DistributiveOmit here (but I wouldn't mind being proven wrong). We need the props defined on the component (BaseProps) to always be present on the resulting type, even if they're not compatible with props defined on C.

[mnajdova]

Do you think the usage of DistributiveOmit here could be another bottleneck?

Nope, it was one of the first things I tried replacing, this is why I left it.

[michaldudak]

If we're going to use these types across the codebase at some point, it would be great to rename the generic props to something more meaningful.

[mnajdova]

I am keeping it as it was before, so that it is visible what exactly are the changes between the two, but yes, we can rename the generics at any point in time :)

Can be done in a dedicated PR where both the existing and the new types will be renamed.

[siriwatknp]

@mnajdova is this new OverridableComponent a POC version or do you aim to release it as well?

[mnajdova]

@mnajdova is this new OverridableComponent a POC version or do you aim to release it as well?

@siriwatknp in order to avoid shipping breaking changes, I am offering this as part of file that can be used as a module augmentation. This way, people that are complaining that the types are slow, can try out to use the new version (that potentially would have a breaking change related to the refs), but it won't be required for everyone to upgrade at the same time. If things go well, we can replace the existing one in v6.

[siriwatknp]

Would this be better?

Suggested change

	& { ref?: React.Ref<Element> }
	& { ref?: C extends keyof JSX.IntrinsicElements ? React.Ref<C> : React.Ref<Element> }

[siriwatknp]

Other than this, looks good to me.

[mnajdova]

Let me compare the time it takes to do the type checks and will report back :)

[mnajdova]

Actually these is a big difference:

~ 100ms for checking App.tsx

vs

~ 1344md for checking App.tsx

I would ship the initial version without this check.

[siriwatknp]

👍

[oliviertassinari]

@mnajdova Great!

Note that I have tried to reproduce the issue raised in #19113, without any success. For example, I took microsoft/TypeScript#34801 without luck. It always takes 2s to show the error, I assume because of microsoft/TypeScript#34801 (comment). So, I used my old Macbook Air 2015, for which it takes longer from restarting the TypeScript server:

to see an error shown

But I saw no measurable differences:

• 5s34

import { Button } from "@mantine/core";

function Demo() {
  return (
    <div>
      <Button component="a" colorr="grayu">Next link button</Button>
    </div>
  );
}

• 6s35

import type {} from "@mui/material/OverridableComponentAugmentation";
import { Button } from "@mui/material";

function Demo() {
  return (
    <div>
      <Button component="a" color="grayu">
        Next link button
      </Button>
    </div>
  );
}

• 6s28

import { Button } from "@mui/material";

function Demo() {
  return (
    <div>
      <Button component="a" color="grayu">
        Next link button
      </Button>
    </div>
  );
}

• 5s13

import Link from "next/link";

function Demo() {
  return (
    <Link href="/hello" passHreff>
      <a>Next link button</a>
    </Link>
  );
}

• 6s03

import React from "react";
import { styled } from "@stitches/react";
import { violet, blackA, red, mauve } from "@radix-ui/colors";

const Button = styled("button", {
  all: "unset",
  display: "inline-flex",
  alignItems: "center",
  justifyContent: "center",
  borderRadius: 4,
  padding: "0 15px",
  fontSize: 15,
  lineHeight: 1,
  fontWeight: 500,
  height: 35,
  variants: {
    variant: {
      violet: {
        backgroundColor: "white",
        color: violet.violet11,
        boxShadow: `0 2px 10px ${blackA.blackA7}`,
        "&:hover": { backgroundColor: mauve.mauve3 },
        "&:focus": { boxShadow: `0 0 0 2px black` },
      },
      red: {
        backgroundColor: red.red4,
        color: red.red11,
        "&:hover": { backgroundColor: red.red5 },
        "&:focus": { boxShadow: `0 0 0 2px ${red.red7}` },
      },
      mauve: {
        backgroundColor: mauve.mauve4,
        color: mauve.mauve11,
        "&:hover": { backgroundColor: mauve.mauve5 },
        "&:focus": { boxShadow: `0 0 0 2px ${mauve.mauve7}` },
      },
    },
  },
  defaultVariants: {
    variant: "violet",
  },
});

const AlertDialogDemo = () => (
  <Button variant="mauvee">
    Cancel
  </Button>
);

export default AlertDialogDemo;

• 7s41

import { Button } from "antd";

export default function Demo() {
  return (
    <Button type="dashed" size="smalls">
      Next link button
    </Button>
  );
}

Maybe we should call it a day, close the issue for lack of clear reproduction.

[mnajdova]

Thanks for taking a look @oliviertassinari. I agree, testing TypeScript perf problems is not an easy thing to do. I also suspect that relaying on VS code's TypeScript server to show the error can have lots of variables.

I will leave a comment on the issue opened, with the instruction for people to try the import to the module augmentation from this PR and will close the issue and ask people if they see problem in the future to create an issue with a repository with reproduction and an output of tsc -generateTrace trace compared with at least default React.

[mnajdova]

The last section here https://github.com/microsoft/TypeScript/wiki/Performance#preferring-base-types-over-unions makes me good about this change :)) cc @michaldudak

[mnajdova]

@eps1lon I thought it was worth mentioning you here. The improvements that you can see in the PR description are marely because of this change:

 export type OverrideProps<
   M extends OverridableTypeMap,
   C extends React.ElementType
 > = (
   & BaseProps<M>
-  & DistributiveOmit<React.ComponentPropsWithRef<C>, keyof BaseProps<M>>
+  & DistributiveOmit<React.ComponentPropsWithoutRef<C>, keyof BaseProps<M>>
+  & { ref?: React.Ref<Element> }
);

Based on this, looks like ComponentPropsWithRef is somewhat of a bottleneck. I understand that we are using looser types now, but maybe it's worth reporting this on the TypeScript repository and wait to see if they have some suggestion on how to improve the type. I am curious to hear your opinion.