[Select] Update renderValue prop's TypeScript type

[ZeeshanTamboli]

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

Closes #34083

We add conditional type on renderValue based on displayEmpty prop. If we don't use conditional type and simply consider renderValue type as (value: T | '') => React.ReactNode, we will always have to check for value not being an empty string to be able to use object methods like map, join etc (Array type) in renderValue.

Instead:

• When displayEmpty is false, renderValue type is (value: T) => React.ReactNode.
• When displayEmpty is true, renderValue type is (value: T | '') => React.ReactNode

Before: https://codesandbox.io/s/jolly-julien-qr1xlr?file=/src/App.tsx

After: https://codesandbox.io/s/staging-surf-fz8eme?file=/src/App.tsx

---

I am hoping this would not be breaking change for devs who use the displayEmpty prop and renderValue prop together requiring them to add a check for value type in renderValue callback - like I had to update the demo.

[mui-bot]

	Messages
📖	Netlify deploy preview: https://deploy-preview-34177--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against d47da06

[mnajdova]

Overall the changes make sense, but I would count this as a breaking change if I am being honest. Let's wait to see what @michaldudak thinks before moving forward.

[michaldudak]

One improvement we could try to minimize the breaking changes is to include the multiple?: false in the ConditionalRenderValueType like this:

type ConditionalRenderValueType<T> =
  | {
      /**
       * If `true`, a value is displayed even if no items are selected.
       *
       * In order to display a meaningful value, a function can be passed to the `renderValue` prop which
       * returns the value to be displayed when no items are selected.
       *
       * ⚠️ When using this prop, make sure the label doesn't overlap with the empty displayed value.
       * The label should either be hidden or forced to a shrunk state.
       * @default false
       */
      displayEmpty?: false;
      /**
       * Render the selected value.
       * You can only use it when the `native` prop is `false` (default).
       *
       * @param {any} value The `value` provided to the component.
       * @returns {ReactNode}
       */
      renderValue?: (value: T) => React.ReactNode;
      /**
       * If `true`, `value` must be an array and the menu will support multiple selections.
       * @default false
       */
      multiple?: boolean;
    }
  | { displayEmpty: true; multiple?: false; renderValue?: (value: T | '') => React.ReactNode }
  | { displayEmpty: true; multiple: true; renderValue?: (value: T) => React.ReactNode };

This way the demo could remain as it was before.

I don't think developers will be affected by the breaking change. If someone was using displayEmpty: true, multiple: false, and a custom renderValue, they had to cast the renderValue parameter anyway to be able to handle '', so their code should still work even after this change.

[ZeeshanTamboli]

@michaldudak I have made your suggested change. It sounds good to me. Please take a look!

[michaldudak]

Could you please also verify the displayEmpty: true; multiple: true case here?

[ZeeshanTamboli]

It's already covered in a demo, but still added it in spec here now incase the demo is removed in future.

[michaldudak]

I think we'll have to copy the JSDocs here as well as they don't show up in VSCode otherwise.

[ZeeshanTamboli]

@michaldudak Can this be reviewed as well?

[michaldudak]

Looks good to me now!

[pcorpet]

FYI this is breaking my code because I was using:

interface IProps extends SelectProps {

for a React component of my own that is wrapping the Select, but now SelectProps is a type and not an interface anymore.

I'll see if I can make it work by switching to a type as well.

[ZeeshanTamboli]

@pcorpet A type can be extended to an interface with the extends keyword if the type satisfies the restrictions. I think it should work. Can you provide a CodeSandbox with a minimal example?

Edit: I can reproduce with the following error:

An interface can only extend an object type or intersection of object types with statically known members.ts(2312)

[jonesmac]

@ZeeshanTamboli @pcorpet I updated an existing issue with the cause here - #35728 (comment)

I'm guessing Mui Component Prop interfaces should never be converted to types since they cannot be extended.

[michaldudak]

@jonesmac it's not a matter of props being a type or an interface. The error comes from the fact that now the shape of SelectProps isn't known at compile time and thus it can't be used to build your own interfaces based on it. You can still, however, build your own types based on it:

type Props = SelectProps & {

However, this does count as a breaking change, and I missed this when reviewing the code. I'm now trying to find a way to fix this while having #34083 resolved.