[typescript] Make types of componentsProps consistent

[michaldudak]

As discussed in #26810, this changes the types of unstyled components' componentsProps fields to Record<string, any>.

This PR sets the type of componentsProps' fields to the props of the matching default components field.

It eliminates the need for module augmentation when the default (or compatible) components are used.

Closes #26810
Closes #27929
Closes #27869

[mui-pr-bot]

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 463bcd0

[oliviertassinari]

Should we update pickers, date picker, and autocomplete too?

[michaldudak]

In Autocomplete we have componentsProps: { clearIndicator } but don't allow customizing components: { ClearIndicator }. I think in this case we can leave more strongly typed componentsProps. I'll update the other cases.

[eps1lon]

What is the point of making the types consistent when it reduces type coverage in some cases?

Consistency is not a target. It's an indicator for other problems. So we need to make sure we understand the problem. Just because something is inconsistent, doesn't mean we need to fix it. For example, apples and bananas are inconsistent fruits. Doesn't mean we need to invent a banana-apple to have consistent fruits.

[michaldudak]

@eps1lon Take a look at the discussion in #26810 for rationale.

I'm not doing this just for the sake of consistency. I believe having Record<string, any> will be easier to use for developers than { as, styleProps } as they won't have to do module augmentation when adding any other prop.

For DX it would be ideal if componentsProps types' were inferred from components, but, as we discussed in #26810, we can't have that, so this is the compromise.

[eps1lon]

The general direction so far has been "strict by default" and let developers loosen up type-checking if they need to. The reverse is usually not possible. This goes against that direction since you can now write <Foo componentProps={{root:{...{className: '', c1assName: ''}}}} /> which the type-checker will never catch. With <Foo componentProps={{root:{...{className: '', c1assName: ''} as any}}} /> it's at least a bit more obvious that componentProps needs to be treated carefully.

I do agree that module augmentation is the wrong tool here since it applies globally.
Being consistent here is not the problem. Though it's unclear here that we do need consistency. The problem is that we don't move in a consistent direction i.e. some components are now looser, some are stricter.

I think we discussed inferring a while back? Can you share the latest progress so that I can poke at it a bit?

[michaldudak]

I think we discussed inferring a while back? Can you share the latest progress so that I can poke at it a bit?

I haven't looked into it much since we talked about it. Just added some type tests in SwitchUnstyled.spec.tsx:
https://github.com/michaldudak/material-ui/tree/feat/typed-slots/packages/material-ui-unstyled/src/SwitchUnstyled

[michaldudak]

@eps1lon have you had a chance to take a look at the code in my feat/typed-slots branch? Do the tests make my intention clearer?

[michaldudak]

We got the first issue about type errors in componentsProps: #27869

I think, at the very least, we can type componentsProps as the props of the default corresponding component.
For example: In SwitchUnstyled, the default Root component is span, so componentsProps.root would have the HTMLAttributes<HTMLSpanElement> & { as?: React.ElementType, ownerState: SwitchState } type.

It would still require casting when something else is provided to components.Root, but in cases where just componentsProps.root is customized, it will work fine and won't require any additional work from developers.

[mnajdova]

I think, at the very least, we can type componentsProps as the props of the default corresponding component.
For example: In SwitchUnstyled, the default Root component is span, so componentsProps.root would have the HTMLAttributes & { as?: React.ElementType, ownerState: SwitchState } type.

I like this.

[oliviertassinari]

@michaldudak Sounds great, and identical to the current tradeoff of the xxxProps and xxxComponent props.

[mnajdova]

@michaldudak we can go with your proposal, there has been a week without a complain, so let's push this forward :)

[michaldudak]

Sure, it's on my to-do list. I'll likely start today.

[michaldudak]

After further thought, I decided to use just HTMLAttributes / ComponentProps of the default component and not augment it with { as, ownerState }. When as property is used, the type of props change, so devs would need to cast or augment modules anyway.

As for ownerState - @mnajdova what's the actual use case for providing it in componentsProps? As I understand it, ownerState is set by the owner, not provided from the outside.

[mnajdova]

As for ownerState - @mnajdova what's the actual use case for providing it in componentsProps? As I understand it, ownerState is set by the owner, not provided from the outside.

Originally, the styleProps were props used for styling, so the component itself, could propagate some props they want to use for styling purposes, for example propagate a color prop as part of the styleProps. We have currently an example of this in the Badge component - https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Badge/Badge.js#L243

If we sent directly this prop to the unstyled component, we would need to also configure somehow that it should not be spread on the DOM elements.

[michaldudak]

We discussed it with @mnajdova off-site and came to the conclusion that adding more properties to ownerState by the styled component isn't the best idea. After all, this is the state of the owner, that is - the unstyled component.
Instead, if any additional props are needed to style the slot (as in the Badge example), we can pass them directly in componentsProps.
These extra props must not be used when a host component is used on a slot, so the final version should look as follows:

 componentsProps={{
   root: {
-    ...componentsProps.root,
-    ...((!components.Root || !isHostComponent(components.Root)) && {
-      ownerState: { ...componentsProps.root?.ownerState, color },
-    }),
+    color: isHostComponent(BadgeRoot) ? undefined : color,
   },

Obviously, this creates another problem with typing the componentsProps. We haven't found anything better than:

export interface BadgeComponentsPropsOverrides {}

// ...

componentsProps: {
  root: React.HTMLAttributes<HTMLSpanElement> & BadgeComponentsPropsOverrides;
  badge: React.HTMLAttributes<HTMLSpanElement> & BadgeComponentsPropsOverrides;
}

This will allow developers to use all the standard HTML attributes out of the box and add custom ones using module augmentation.

[mnajdova]

export interface BadgeComponentsPropsOverrides {}

// ...

componentsProps: {
  root: React.HTMLAttributes<HTMLSpanElement> & BadgeComponentsPropsOverrides;
  badge: React.HTMLAttributes<HTMLSpanElement> & BadgeComponentsPropsOverrides;
}

I like that this is strict by default, but can be augmented to accept more props 👍

[mnajdova]

@michaldudak one quesiton. Should we maybe use just React.HTMLAttributes<HTMLElement>? I am asking because developers may provide components={{ Root: 'div' }}. I am not sure..

 componentsProps: {
-  root: React.HTMLAttributes<HTMLSpanElement> & BadgeComponentsPropsOverrides;
-  badge: React.HTMLAttributes<HTMLSpanElement> & BadgeComponentsPropsOverrides;
+  root: React.HTMLAttributes<HTMLElement> & BadgeComponentsPropsOverrides;
+  badge: React.HTMLAttributes<HTMLElement> & BadgeComponentsPropsOverrides;
 }

[michaldudak]

HTMLElement, HTMLDivElement, HTMLSpanElement and many others are actually equivalent in this context.
See https://codesandbox.io/s/nervous-smoke-skqyq

[michaldudak]

@oliviertassinari, @mnajdova, @eps1lon Is there anything else you'd like to see changed, or is this good to go? I think it's worth merging it before the next RC.