[core] Adds component prop to OverrideProps type

[sai6855]

closes: #27703
closes #32392
closes #32872
closes #15827
closes #33339
closes #29942
closes #29191

This PR Adds component prop to OverrideProps

IMPORTANT CHANGE: https://github.com/mui/material-ui/pull/35924/files#diff-02789dd73696f8809fa6d0f1c92b094b5bfe1336896dfa5b6a99e7401fe0db44

Tests for #27703: https://github.com/mui/material-ui/pull/35924/files#diff-bb4c51a3c2eb1c21aee05d85ff711b43109ca2ace91ea48bce9b91271806dcf4, https://github.com/mui/material-ui/pull/35924/files#diff-253d444cc99d27cd1ec9995eccfedab87fd2e9494e6ef52eb1d9c5985c435537

Tests for #32392 : https://github.com/mui/material-ui/pull/35924/files#diff-b3f0ad755111cc9fcaf7c5093cc5cbbd82ed0c8a3a2cc115145d6e0a61f2b8fd

Tests for #32872 : https://github.com/mui/material-ui/pull/35924/files#diff-435b537caac0b4be4d31544b02848de31a25a0bf3e06a6d4e10a5d48f98d8000

Tests for #15827 : https://github.com/mui/material-ui/pull/35924/files#diff-ea054e0fb385828d56c70e70509fafd9dca12c09ffc5e8a5e8a427df0d9e3fd1

Tests for #33339 : https://github.com/mui/material-ui/pull/35924/files#diff-0beec8e9cb1664a1bdac23df7fb83e4de30a162a0fb7f69993c50e5d3d016718

Tests for #29942 : https://github.com/mui/material-ui/pull/35924/files#diff-2d7ab66407884c6bd2edc52e35793039196e22f676e9e8b53bc995a14e66e673

Tests for #29191 :https://github.com/mui/material-ui/pull/35924/files#diff-3965b5dc7b5d4ea3096b0efe626dc9a7bd64a66d6c2f895b0e179ef8b01bd5aa

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

• docs/data/material/guides/routing/routing.md

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 489d7b2

[michaldudak]

As I understand it, the root cause of this problem is that we use the same type - OverrideProps for two things: to define the props of the component when a component prop is used (as in OverridableComponent) and to specify types of props in individual components (as type ButtonBaseProps<D, P> = OverrideProps<ButtonBaseTypeMap<P, D>, D>;).
Looking at the shape of the OverrideProps, only the first usage is correct, and ideally, we should use a different type for the props themselves.

However, the change you made fixes the problem of the second case and shouldn't affect the first case (because the OverrdableComponent's props become { component: C } & { component?: C } (plus other members), which is equivalent to { component: C }.

I can't think of every possible way developers use these types, so I can't predict how safe this change is. I think it should work fine, but I'm far from being sure. I'd appreciate a second reviewer's thoughts here.

[michaldudak]

This is unfortunate. We shouldn't recommend such hacks, ideally, but I understand where it's coming from. Could ou try to work around it so such an ugly cast is not necessary?

[sai6855]

i've tried to remove as unknown type without disturbing other files but couldn't able to do it. So instead of fixing casting problem, tried to fix (commit 089f989 ) the root problem which lies in this file .

If you are fine with converting props in props.d.ts file to generic, i'll convert all components to generic where applicable. if not i'd appreciate your help fixing the casting issue.

[michaldudak]

I'd ask @mnajdova about the generic, as she's overseeing the styles. To me, it feels OK, but I have less context about the usage of these types.

[michaldudak]

Suggested change

	& {
	/**
	* The component used for the root node.
	* Either a string to use a HTML element or a component.
	*/
	component?: C
	}
	& {
	/**
	* The component used for the root node.
	* Either a string to use a HTML element or a component.
	*/
	component?: C
	}

[sai6855]

Resolved in this commit 132e413

[michaldudak]

All right, this looks good in general. Please merge in the latest mater, and I'll do the final review afterward.

[ZeeshanTamboli]

I like it, but I didn't understand why this change had to be done. The tests give me confidence to proceed. Great job!

[DiegoAndai]

Hey @sai6855, thanks for working on this, and sorry for chiming in so late

I think @michaldudak and @ZeeshanTamboli approvals are enough to merge. Anyway, I also reviewed it and have some questions, if you could answer these before merging I would appreciate it.

PD: Regarding this comment: #35924 (review). I'll start a discussion with @mui/core about what we can do to improve it.

[DiegoAndai]

LGTM! 🚀

[oliviertassinari]

So, we can remove this now?

material-ui/packages/mui-material/src/OverridableComponent.d.ts

Lines 16 to 22 in 9a6533b

	props: {
	/**
	* The component used for the root node.
	* Either a string to use a HTML element or a component.
	*/
	component: C;
	} & OverrideProps<M, C>,

and

material-ui/packages/mui-base/src/Badge/Badge.types.ts

Lines 90 to 92 in 9a6533b

	> = OverrideProps<BadgeTypeMap<{}, RootComponentType>, RootComponentType> & {
	component?: RootComponentType;
	};

and many more?

[sai6855]

= OverrideProps<BadgeTypeMap<{}, RootComponentType>, RootComponentType> & {
component?: RootComponentType;
};

base-ui uses OverrideProps from mui-types package. related file - https://github.com/mui/material-ui/blob/master/packages/mui-types/index.d.ts#L116

but this PR added component type to OverrideProps in this file https://github.com/mui/material-ui/blob/master/packages/mui-material/src/OverridableComponent.d.ts#L31

So i don't think above change is required.

[sai6855]

props: { 

   /** 
    * The component used for the root node. 
    * Either a string to use a HTML element or a component. 
    */ 
   component: C; 
 } & OverrideProps<M, C>, 

regarding removal of this code - i did a quick check and removed that type, but i can see typescript errors in few files. i'll have to dig deeper on why that's the case

[Methuselah96]

@sai6855 @michaldudak @oliviertassinari FYI, this made #34068 worse. Removing the { component: C } lines from the OverridableComponent type would help, but as @sai6855 noted, removing those lines would make the inference of parameters for callbacks not work for some reason.

[jocelynn1uphealth]

Hey MUI contributors @sai6855 @michaldudak @oliviertassinari! Thanks so much for all you do. This change is giving me new type errors when I pass a component prop into Card, a la <Card component="form">. The TypeError says I can only have "div" or undefined as values. Anything I should be doing differently in light of this change?

ETA: patch version 5.14.2 fixed this !

[sai6855]

Can you please share codesandbox link which reproduces the issue . Cc @jocelynn1uphealth

[jocelynn1uphealth]

@sai6855 the patch fix this morning fixed the issue, now it's working again. Thanks!