Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Omit in Typescript to remove props from end user breaks component={} prop and type inference #35026

Open
2 tasks done
jannisg opened this issue Nov 6, 2022 · 3 comments
Assignees
Labels
bug 🐛 Something doesn't work typescript

Comments

@jannisg
Copy link

jannisg commented Nov 6, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/wandering-darkness-mevzqv?file=/src/App.tsx

Example shows this behaviour for a polymorphic component (Button) and a simpler component, Select.

Steps:

  1. Import MUI Component and its component prop type, i.e. Select and SelectProps
  2. Use Typescript's Omit function to remove an optional prop, Omit<SelectProps, "defaultOpen">
  3. Export a new wrapper component with the newly created type from Step 2
  4. Watch the TypeScript types breaking by either re-enables the removed prop when using component="a" on Button or loosing type inference of the Select value where event.target.value becomes unknown instead of string (see example)

Current behavior 😯

Removing props from the Typescript interface, breaks general types applying to the modified component.

For polymorphic components that support the component="x" prop, setting it will re-enable the Omit'ed prop.

For other components, the components type inference breaks and event handler values result in unknown instead of string or its appropriate type.

Expected behavior 🤔

I would like to remove some of the defined props or prop values from the typescript types for components without breaking other typed functionality.

If I choose to Omit a prop, the other props should continue working as normal.

Context 🔦

We are using Material UI Core (and MUI X Pro) to build an in-house Design System.

We want to make minimal changes/simplifications to the component types to guide our end-users towards using the components as we intend them to be used.

Material UI provides a rich interface of custom component options via its props and often times we would like to curate which props are available to our internal development teams that use our library.

In order to do so, our intend is to re-export MUI components with modified typescript types which "hide" the props we don't want our users to set/use.

Examples

Examples for Button and Select of this are below:

import MuiButton from '@mui/material/Button';
import type { ButtonProps as MuiButtonProps } from '@mui/material/Button';

type ExcludedMuiProps =
  | 'disableFocusRipple'
  | 'disableElevation'
  | 'disableRipple'
  | 'color';

interface ButtonProps extends Omit<MuiButtonProps, ExcludedMuiProps> {
  color?: Exclude<MuiButtonProps['color'], 'inherit'>;
}

export type { ButtonProps };

export const Button: React.FC<ButtonProps> = MuiButton;

This should result in 3 props not being suggested in the typescript intellisense:

  • disableFocusRipple
  • disableElevation
  • disableRipple

And should remove the inherit option from the color prop.

import MuiSelect from '@mui/material/Select';
import type { SelectProps as MuiSelectProps } from '@mui/material/Select';

type ExcludedMuiProps = 'defaultOpen' | 'IconComponent' | 'displayEmpty';

interface SelectProps extends Omit<MuiSelectProps, ExcludedMuiProps> {
  component?: React.ElementType;
}

export type { SelectProps };
export const Select: React.FC<SelectProps> = MuiSelect;

Wrong approach?

If this is the wrong approach for curating which props or which prop values are available to our development teams, I would love to learn the idiomatic way to do this as we really want to limit the scope of our end users (internal) to set props we don't want to support/use for brand consistency.

Your environment 🌎

npx @mui/envinfo
  System:
    OS: macOS 12.5.1
  Binaries:
    Node: 16.18.0 - ~/.nvm/versions/node/v16.18.0/bin/node
    Yarn: 3.2.1 - ~/.nvm/versions/node/v16.18.0/bin/yarn
    npm: 8.19.2 - ~/.nvm/versions/node/v16.18.0/bin/npm
  Browsers:
    Chrome: 107.0.5304.87
    Edge: Not Found
    Firefox: Not Found
    Safari: 15.6.1
  npmPackages:
    @emotion/react: ^11.5.0 => 11.10.4 
    @emotion/styled: ^11.3.0 => 11.10.4 
    @mui/base:  5.0.0-alpha.96 
    @mui/core-downloads-tracker:  5.10.4 
    @mui/icons-material: ^5.10.3 => 5.10.3 
    @mui/material: ^5.10.4 => 5.10.4 
    @mui/private-theming:  5.10.3 
    @mui/styled-engine:  5.10.4 
    @mui/system:  5.10.4 
    @mui/types:  7.2.0 
    @mui/utils:  5.10.3 
    @mui/x-date-pickers:  5.0.5 
    @mui/x-date-pickers-pro: ^5.0.5 => 5.0.5 
    @mui/x-license-pro:  5.16.0 
    @types/react: 18.0.14 => 18.0.14 
    react: ~18 => 18.2.0 
    react-dom: ~18 => 18.2.0 
    typescript: 4.7.4 => 4.7.4 

npx tsc --showConfig
{
  "compilerOptions": {
    "rootDir": "../..",
    "sourceMap": true,
    "declaration": false,
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "resolveJsonModule": true,
    "importHelpers": true,
    "target": "es6",
    "module": "esnext",
    "lib": [
      "es2017",
      "dom"
    ],
    "skipLibCheck": true,
    "skipDefaultLibCheck": true,
    "baseUrl": "../..",
    "paths": {
      "@ds/carousel": [
        "libs/carousel/src/index.ts"
      ],
      "@ds/core": [
        "libs/ui/src/index.ts"
      ]
    },
    "jsx": "react-jsx",
    "jsxImportSource": "@emotion/react",
    "allowJs": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "noImplicitOverride": true,
    "noPropertyAccessFromIndexSignature": true,
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true
  },
  "references": [
    {
      "path": "./tsconfig.lib.json"
    },
    {
      "path": "./tsconfig.spec.json"
    }
  ],
  "exclude": [
    "../../node_modules",
    "../../tmp"
  ]
}
@jannisg jannisg added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 6, 2022
@jannisg jannisg changed the title Using Omit in Typescript to remove props from end user breaks component and type inference Using Omit in Typescript to remove props from end user breaks component={} prop and type inference Nov 6, 2022
@michaldudak michaldudak self-assigned this Nov 8, 2022
@michaldudak
Copy link
Member

The problem seems to come from the lack of a component prop in the ButtonProps (and the OverrideProps type in general). I don't know why exactly it was implemented this way but just adding it isn't that simple.
We have a couple of issues in Typescript area. We'll have to review them at some point and decide which are fixable.
For now, you can work around the component prop issue by defining:

type MyButtonProps = Omit<ButtonProps, 'disableFocusRipple' | 'disableRipple'> & {
  component?: React.ElementType;
};

As for the second problem - why isn't the onChange parameter inferred correctly?
SelectProps is a generic type (the generic parameter defines the type of the value). It's defined as SelectProps<T = unknown>, so when you create your own type based on it and don't provide the generic parameter, it defaults to unknown.
Define your props type and MySelect like this to avoid this problem:

type MySelectProps<T> = Omit<SelectProps<T>, 'defaultOpen'>;
const MySelect = function<T>(props: MySelectProps<T>) {
  return <Select {...props} />;
};

@michaldudak michaldudak added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 11, 2022
@jannisg
Copy link
Author

jannisg commented Nov 15, 2022

@michaldudak Thank you for your reply and looking into this.

I have managed to address the Select component but am still having issues with the other suggestion which involved passing in an element via the component={} prop and the resulting wrapper correctly inferring the types of the given components.

I have created an isolated example using a wrapped Container element with a custom Surface component that is being passed in, in a new sandbox here: https://codesandbox.io/s/kind-lewin-8tvr5l?file=/src/App.tsx

I think the complication stems from the fact that the ContainerProps actually require

export type ContainerProps<
  D extends React.ElementType = ContainerTypeMap['defaultComponent'],
  P = {}>

and with me applying the suggested fix for Button from above as:

// NOTE: Is `any` the right approach here?
//       If not, how do I get the element type that was given to `component={}` here?
type MyContainerProps<T> = Omit<ContainerProps<any, T>, "disableGutters"> & {
  component?: React.ElementType;
};
const MyContainer = function <T>(props: MyContainerProps<T>) {
  return <Container {...props} />;
};

In the following usage, the props from Surface are not available on MyContainer.

{/* NOTE: The `elevation` prop is not working as expected, it should throw an error */}
<MyContainer maxWidth="sm" component={Surface} elevation={"test"}>
    A container without the 'disableGutters' prop.
    The props of the `component` given are not working as expected.
</MyContainer>

The Surface component (as per the linked CodeSandbox) is defined as:

interface SurfaceProps {
  elevation?: number;
  emphasis?: "low" | "medium" | "high";
}
const Surface = styled("div")<SurfaceProps>(
  ({ theme, elevation = 1, emphasis }) => {
    const colorMap: Record<SurfaceProps["emphasis"], string> = {
      low: "green",
      medium: "blue",
      high: "red"
    };

    return {
      backgroundColor: emphasis ? colorMap[emphasis] : "white",
      boxShadow: `0px 2px ${elevation * 2}px black`,
      padding: theme.spacing(2),
      borderRadius: theme.shape.borderRadius
    };
  }
);

If you have any ideas on where I am going wrong with this, I would appreciate your help once again :)

@michaldudak
Copy link
Member

In this case, IMO, your best bet would be to define the type of the component and its props the same way we do in Material UI itself. We use so-called "type maps" to define the props and default root component a given component uses and pass them to utility types: OverridableComponent and OverrideProps that take care of all the magic associated with polymorphic components (see how they are defined in Container: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Container/Container.d.ts).

For a concrete example, I've forked your codesandbox and modified the types: https://codesandbox.io/s/confident-sun-652mrk?file=/src/App.tsx

I understand it's not perfect and requires a fair share of typing but modeling polymorphic components in TypeScript is not a trivial task.

Let me know if it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work typescript
Projects
None yet
Development

No branches or pull requests

3 participants