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

[material-ui] Incorrect React component prop children types for components like AvatarGroup #38533

Open
2 tasks done
jaydenseric opened this issue Aug 18, 2023 · 1 comment
Open
2 tasks done
Assignees
Labels
component: avatar This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation

Comments

@jaydenseric
Copy link
Contributor

jaydenseric commented Aug 18, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Many of the Material UI React components that expect particular React elements in the prop children currently have an incorrect TypeScript type for the prop of React.ReactNode.

In demo.tsx:

import Avatar from "@mui/material/Avatar";
import AvatarGroup from "@mui/material/AvatarGroup";

function Demo() {
  return (
    <AvatarGroup>
      <>
        <Avatar>AA</Avatar>
        <Avatar>BB</Avatar>
      </>
    </AvatarGroup>
  );
}

Current behavior 😯

There will not be a TypeScript (or prop-types) error about using a React Fragment in the AvatarGroup prop children, but if you render the Demo component this will be logged as an error in the browser console:

MUI: The AvatarGroup component doesn't accept a Fragment as a child.
Consider providing an array instead.
Screenshot 2023-08-18 at 10 39 08 am

Expected behavior 🤔

For the current AvatarGroup API that intentionally forbids using a React Fragment in the prop children, there should be a TypeScript error that a React Fragment was used in the AvatarGroup prop children, and there should also be a prop-types warning in the browser console about the value used.

Ideally the AvatarGroup API (and the API for other components with similar expectations about React Elements in children) would be changed to allow using a React.ReactNode type value in the prop children, including a React Fragment. This could be achieved by implementing something like react-keyed-flatten-children to convert children into a flattened and keyed array, accounting for React Fragments.

Context 🔦

Here is an example where AvatarGroup has the wrong TypeScript type for the React component prop children, resulting also in the React component propTypes being incorrect…

Source for the type:

/**
* The avatars to stack.
*/
children?: React.ReactNode;

Published type:

https://unpkg.com/browse/@mui/[email protected]/AvatarGroup/AvatarGroup.d.ts

Resulting incorrect generated propTypes:

/**
* The avatars to stack.
*/
children: PropTypes.node,

Incorrect API docs:

https://mui.com/material-ui/api/avatar-group/#AvatarGroup-prop-children

Screenshot 2023-08-18 at 10 24 46 am

Some searches to try to help find other React components with an incorrect prop type for children:

Note that the current limitation of the prop children not accepting a React component for React components like AvatarGroup is particularly annoying when writing Storybook stories, when you want to provide the children via the story config args.children. For example, in AvatarGroup.stories.tsx:

import Avatar from "@mui/material/Avatar";
import AvatarGroup from "@mui/material/AvatarGroup";
import type { Meta, StoryObj } from "@storybook/react";

const meta = {
  component: AvatarGroup,
} satisfies Meta<typeof AvatarGroup>;

export default meta;

type Story = StoryObj<typeof meta>;

export const ChildrenFragment = {
  args: {
    children: (
      <>
        <Avatar>AA</Avatar>
        <Avatar>BB</Avatar>
      </>
    ),
  },
} satisfies Story;

export const ChildrenArray = {
  args: {
    children: [<Avatar>AA</Avatar>, <Avatar>BB</Avatar>],
  },
} satisfies Story;

The first story ChildrenFragment at render results in this console error:

MUI: The AvatarGroup component doesn't accept a Fragment as a child.
Consider providing an array instead.

While the second story ChildrenArray (which is what a user might attempt after following the instructions of the first console error to provide an array instead) at render results in this console error:

wrapComponent.tsx:33 Warning: Each child in a list should have a unique "key" prop.

Check the render method of `AvatarGroup`. See https://reactjs.org/link/warning-keys for more information.
    at Avatar
    at AvatarGroup
    at …

The Material UI component docs for the AvatarGroup prop children didn't give any indication there would be render issues for stories written like this, there was no build time type safety, and it's not clear to a user after manually trying to render the components and reading the console errors the best way to resolve it.

Your environment 🌎

npx @mui/envinfo
  System:
    OS: macOS 13.5
  Binaries:
    Node: 20.5.0 - ~/.volta/tools/image/node/20.5.0/bin/node
    Yarn: 1.22.19 - ~/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 9.8.0 - ~/.volta/tools/image/node/20.5.0/bin/npm
  Browsers:
    Chrome: 116.0.5845.96
    Edge: 115.0.1901.203
    Safari: 16.6
  npmPackages:
    @emotion/react:  11.11.1 
    @emotion/styled: ^11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.11 
    @mui/core-downloads-tracker:  5.14.5 
    @mui/icons-material: ^5.14.3 => 5.14.3 
    @mui/material: ^5.14.5 => 5.14.5 
    @mui/private-theming:  5.14.5 
    @mui/styled-engine:  5.13.2 
    @mui/system:  5.14.5 
    @mui/types:  7.2.4 
    @mui/utils:  5.14.5 
    @types/react: ^18.2.20 => 18.2.20 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    typescript: ^5.1.6 => 5.1.6
@jaydenseric jaydenseric added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 18, 2023
@zannager zannager added the component: avatar This is the name of the generic UI component, not the React module! label Aug 18, 2023
@samuelsycamore
Copy link
Member

Thanks for this detailed report @jaydenseric! It sounds like the best solution for now would be to document this use case to let others know about the potential pitfalls.

@samuelsycamore samuelsycamore added docs Improvements or additions to the documentation and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 12, 2023
@samuelsycamore samuelsycamore changed the title Incorrect React component prop children types for components like AvatarGroup [material-ui] Incorrect React component prop children types for components like AvatarGroup Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: avatar This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

No branches or pull requests

4 participants