[core] Offer alternative to OverridableComponent via module augmentation for better performance
#32735
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,44 @@ | ||||||
| import * as React from 'react'; | ||||||
| import { DistributiveOmit } from '@mui/types'; | ||||||
|
|
||||||
| declare module '@mui/material/OverridableComponent' { | ||||||
| /** | ||||||
| * A component whose root component can be controlled via a `component` prop. | ||||||
| * | ||||||
| * Adjusts valid props based on the type of `component`. | ||||||
| */ | ||||||
| interface OverridableComponent<M extends OverridableTypeMap> { | ||||||
| <C extends React.ElementType>( | ||||||
| props: { | ||||||
| /** | ||||||
| * The component used for the root node. | ||||||
| * Either a string to use a HTML element or a component. | ||||||
| */ | ||||||
| component: C; | ||||||
| } & OverridePropsVer2<M, C>, | ||||||
| ): JSX.Element; | ||||||
| (props: DefaultComponentPropsVer2<M>): JSX.Element; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Props of the component if `component={Component}` is used. | ||||||
| */ | ||||||
| // prettier-ignore | ||||||
| type OverridePropsVer2< | ||||||
| M extends OverridableTypeMap, | ||||||
| C extends React.ElementType, | ||||||
| > = ( | ||||||
| & BaseProps<M> | ||||||
| & DistributiveOmit<React.ComponentPropsWithoutRef<C>, keyof BaseProps<M>> | ||||||
|
There was a problem hiding this comment. Do you think the usage of 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. There was a problem hiding this comment. I don't think we can get away without There was a problem hiding this comment.
Nope, it was one of the first things I tried replacing, this is why I left it. |
||||||
| & { ref?: React.Ref<Element> } | ||||||
|
There was a problem hiding this comment. Would this be better?
Suggested change
There was a problem hiding this comment. Other than this, looks good to me. There was a problem hiding this comment. Let me compare the time it takes to do the type checks and will report back :) There was a problem hiding this comment. Actually these is a big difference:
vs
I would ship the initial version without this check. |
||||||
| ); | ||||||
|
|
||||||
| /** | ||||||
| * Props if `component={Component}` is NOT used. | ||||||
| */ | ||||||
| // prettier-ignore | ||||||
| type DefaultComponentPropsVer2<M extends OverridableTypeMap> = | ||||||
| & BaseProps<M> | ||||||
| & DistributiveOmit<React.ComponentPropsWithoutRef<M['defaultComponent']>, keyof BaseProps<M>> | ||||||
| & { ref?: React.Ref<Element> }; | ||||||
|
There was a problem hiding this comment. The last section here https://github.com/microsoft/TypeScript/wiki/Performance#preferring-base-types-over-unions makes me good about this change :)) cc @michaldudak |
||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| { | ||
| "extends": "../../tsconfig", | ||
| "include": ["src/**/*", "test/**/*"], | ||
| "exclude": ["test/typescript/moduleAugmentation", "src/OverridableComponentAugmentation.ts"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import * as React from 'react'; | ||
|
|
||
| declare module '@mui/types' { | ||
| /** | ||
| * A component whose root component can be controlled via a `component` prop. | ||
| * | ||
| * Adjusts valid props based on the type of `component`. | ||
| */ | ||
| interface OverridableComponent<M extends OverridableTypeMap> { | ||
| <C extends React.ElementType>( | ||
| props: { | ||
| /** | ||
| * The component used for the root node. | ||
| * Either a string to use a HTML element or a component. | ||
| */ | ||
| component: C; | ||
| } & OverridePropsVer2<M, C>, | ||
| ): JSX.Element; | ||
| (props: DefaultComponentPropsVer2<M>): JSX.Element; | ||
| propTypes?: any; | ||
| } | ||
|
|
||
| /** | ||
| * Props of the component if `component={Component}` is used. | ||
| */ | ||
| // prettier-ignore | ||
| type OverridePropsVer2< | ||
| M extends OverridableTypeMap, | ||
| C extends React.ElementType | ||
| > = ( | ||
| & BaseProps<M> | ||
| & DistributiveOmit<React.ComponentPropsWithoutRef<C>, keyof BaseProps<M>> | ||
| & { ref?: React.Ref<Element> } | ||
| ); | ||
|
|
||
| /** | ||
| * Props if `component={Component}` is NOT used. | ||
| */ | ||
| // prettier-ignore | ||
| type DefaultComponentPropsVer2<M extends OverridableTypeMap> = | ||
| & BaseProps<M> | ||
| & DistributiveOmit<React.ComponentPropsWithoutRef<M['defaultComponent']>, keyof BaseProps<M>> | ||
| & { ref?: React.Ref<Element> }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| { | ||
| "extends": "../../tsconfig", | ||
| "exclude": ["OverridableComponentAugmentation.ts"] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.