Material-ui: Container component does not accept React.ReactNode typed children

[bkiac]

The Container component does not allow null or undefined as children since v4.10.2.

My component:

import React from "react";
import Container from "@material-ui/core/Container";

import Header from "./Header";

const Page: React.FC = ({ children }) => {
  return (
    <>
      <Header />
      <Container>{children}</Container>
    </>
  );
};

export default Page;

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Does not allow null or undefined children with this error message:

src/components/Page.tsx:10:45 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: { component: ElementType<any>; } & { children: string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal; disableGutters?: boolean | undefined; fixed?: boolean | undefined; maxWidth?: false | ... 5 more ... | undefined; } & CommonProps<...> & Pick<...>): Element', gave the following error.
    Type 'ReactNode' is not assignable to type 'string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal'.
      Type 'undefined' is not assignable to type 'string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal'.
  Overload 2 of 2, '(props: DefaultComponentProps<ContainerTypeMap<{}, "div">>): Element', gave the following error.
    Type 'ReactNode' is not assignable to type 'string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal'.
      Type 'undefined' is not assignable to type 'string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal'.

10       <Container>{children}</Container>
                                               ~~~~~~~~~~

  node_modules/@material-ui/core/Container/Container.d.ts:6:5
    6     children: NonNullable<React.ReactNode>;
          ~~~~~~~~
    The expected type comes from property 'children' which is declared here on type 'IntrinsicAttributes & { component: ElementType<any>; } & { children: string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... 1 more ... | (new (props: any) => Component<...>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal; disableGu...'
  node_modules/@material-ui/core/Container/Container.d.ts:6:5
    6     children: NonNullable<React.ReactNode>;
          ~~~~~~~~
    The expected type comes from property 'children' which is declared here on type 'IntrinsicAttributes & { children: string | number | boolean | {} | ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | ReactNodeArray | ReactPortal; disableGutters?: boolean | undefined; fixed?:...'


Found 1 error.

Expected Behavior 🤔

Maybe the API and/or use case has changed, but I think it should accept null or undefined as children.

Context 🔦

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.11.0 |
| React | 16.13.1 |
| TypeScript | 3.9.6 |

[oliviertassinari]

Do you have a live reproduction with v5.0.0-alpha.2?

[oliviertassinari]

Actually, it's probably coming from: https://github.com/mui-org/material-ui/blob/d1287f1874dffeade600364b36727fa7878e9a75/packages/material-ui/src/Container/Container.d.ts#L6

Do we need force a children prop?

[eps1lon]

We always warned at runtime if nullable children were passed: https://github.com/mui-org/material-ui/pull/14499

[oliviertassinari]

It seems we have been progressively easing this constraint. Do you want to keep it? I don't mind either way.

[oliviertassinari]

Linking https://github.com/mui-org/material-ui/pull/21368#discussion_r442640637 as we had a similar discussion started there.

[oliviertassinari]

Looking at the current usage of NonNullable, it seems that the direction that make sense is:

diff --git a/packages/material-ui/src/Container/Container.d.ts b/packages/material-ui/src/Container/Container.d.ts
index e22182fd7..2a7734d2c 100644
--- a/packages/material-ui/src/Container/Container.d.ts
+++ b/packages/material-ui/src/Container/Container.d.ts
@@ -3,7 +3,7 @@ import { OverridableComponent, OverrideProps } from '../OverridableComponent';

 export interface ContainerTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P & {
-    children: NonNullable<React.ReactNode>;
+    children?: React.ReactNode;
     /**
      * If `true`, the left and right padding is removed.
      */

[eps1lon]

it seems that the direction that make sense is:

What changed compared to the initial implementation of Container?

[oliviertassinari]

@eps1lon Nothing has changed in the component itself. The motivation around this move would be to consolidate around one approach, using hindsight. The usage of .isRequired for the children seems to be arbitrary:

• Why is it used in Container, Popper, Fab, GridList, Modal, Tooltip, ClickAwayListener, and InputAdornment?
• Why isn't it used in AppBar, Button, Breadcrumbs, ButtonGroup, Card, Dialog, Hidden, Menu, Portal, Alert, TreeView, AvatarGroup?

Are these components so different they justify using different typing? Not that much. It also seems that people value flexibility.

What do you think?

[l3m]

Apparently, this also lead to changes in docs:

https://v4-9-14.material-ui.com/api/container/ contains children , but
https://v4-10-2.material-ui.com/api/container/ (and 4.11) doesn't list it anymore.

[oliviertassinari]

I would propose the following: we limit the usage of .isRequired and it's NonNullable equivalent to the only cases where a missing children prop would create a problem. For instance, it could create a JavaScript runtime exception or lead to a broken UI.

In practice, this would mean:

diff --git a/packages/material-ui/src/Container/Container.d.ts b/packages/material-ui/src/Container/Container.d.ts
index e22182fd7c..2a7734d2c6 100644
--- a/packages/material-ui/src/Container/Container.d.ts
+++ b/packages/material-ui/src/Container/Container.d.ts
@@ -3,7 +3,7 @@ import { OverridableComponent, OverrideProps } from '../OverridableComponent';

 export interface ContainerTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P & {
-    children: NonNullable<React.ReactNode>;
+    children?: React.ReactNode;
     /**
      * If `true`, the left and right padding is removed.
      */
diff --git a/packages/material-ui/src/Container/Container.js b/packages/material-ui/src/Container/Container.js
index 6471b3289e..1dbfe4cb22 100644
--- a/packages/material-ui/src/Container/Container.js
+++ b/packages/material-ui/src/Container/Container.js
@@ -103,7 +103,7 @@ Container.propTypes = {
   /**
    * @ignore
    */
-  children: PropTypes /* @typescript-to-proptypes-ignore */.node.isRequired,
+  children: PropTypes.node,
   /**
    * Override or extend the styles applied to the component.
    * See [CSS API](#css) below for more details.
diff --git a/packages/material-ui/src/Fab/Fab.d.ts b/packages/material-ui/src/Fab/Fab.d.ts
index f4acfd8fb1..146adfce81 100644
--- a/packages/material-ui/src/Fab/Fab.d.ts
+++ b/packages/material-ui/src/Fab/Fab.d.ts
@@ -7,7 +7,7 @@ export type FabTypeMap<P = {}, D extends React.ElementType = 'button'> = ExtendB
     /**
      * The content of the button.
      */
-    children: NonNullable<React.ReactNode>;
+    children?: React.ReactNode;
     /**
      * The color of the component. It supports those theme colors that make sense for this component.
      */
diff --git a/packages/material-ui/src/Fab/Fab.js b/packages/material-ui/src/Fab/Fab.js
index ef5212b9b2..93e69bf9e7 100644
--- a/packages/material-ui/src/Fab/Fab.js
+++ b/packages/material-ui/src/Fab/Fab.js
@@ -168,7 +168,7 @@ Fab.propTypes = {
   /**
    * The content of the button.
    */
-  children: PropTypes /* @typescript-to-proptypes-ignore */.node.isRequired,
+  children: PropTypes.node,
   /**
    * Override or extend the styles applied to the component.
    * See [CSS API](#css) below for more details.
diff --git a/packages/material-ui/src/InputAdornment/InputAdornment.d.ts b/packages/material-ui/src/InputAdornment/InputAdornment.d.ts
index ac1df8c5c1..7d70cc16b6 100644
--- a/packages/material-ui/src/InputAdornment/InputAdornment.d.ts
+++ b/packages/material-ui/src/InputAdornment/InputAdornment.d.ts
@@ -6,7 +6,7 @@ export interface InputAdornmentTypeMap<P = {}, D extends React.ElementType = 'di
     /**
      * The content of the component, normally an `IconButton` or string.
      */
-    children: NonNullable<React.ReactNode>;
+    children?: React.ReactNode;
     /**
      * Disable pointer events on the root.
      * This allows for the content of the adornment to focus the input on click.
diff --git a/packages/material-ui/src/InputAdornment/InputAdornment.js b/packages/material-ui/src/InputAdornment/InputAdornment.js
index 690ab30579..48c7b4fc64 100644
--- a/packages/material-ui/src/InputAdornment/InputAdornment.js
+++ b/packages/material-ui/src/InputAdornment/InputAdornment.js
@@ -105,7 +105,7 @@ InputAdornment.propTypes = {
   /**
    * The content of the component, normally an `IconButton` or string.
    */
-  children: PropTypes /* @typescript-to-proptypes-ignore */.node.isRequired,
+  children: PropTypes.node,
   /**
    * Override or extend the styles applied to the component.
    * See [CSS API](#css) below for more details.
diff --git a/packages/material-ui/src/Popper/Popper.d.ts b/packages/material-ui/src/Popper/Popper.d.ts
index e4a23649ba..6c237e3693 100644
--- a/packages/material-ui/src/Popper/Popper.d.ts
+++ b/packages/material-ui/src/Popper/Popper.d.ts
@@ -29,7 +29,7 @@ export interface PopperProps extends Omit<React.HTMLAttributes<HTMLDivElement>,
   /**
    * Popper render function or node.
    */
-  children:
+  children?:
     | React.ReactNode
     | ((props: {
         placement: PopperPlacementType;
diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index 35eb2e11ef..f0604d19b4 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -280,7 +280,7 @@ Popper.propTypes = {
   children: PropTypes /* @typescript-to-proptypes-ignore */.oneOfType([
     PropTypes.node,
     PropTypes.func,
-  ]).isRequired,
+  ]),
   /**
    * A HTML element, component instance, or function that returns either.
    * The `container` will have the portal children appended to it.

[suliskh]

Yeah, should it be applied to the other components you mention earlier? (i.e. Popper, Fab, GridList, Modal, Tooltip, ClickAwayListener, and InputAdornment)?

[oliviertassinari]

Only to the ones that crash without the prop.

[suliskh]

Has anyone working on the PR? May I?

[eps1lon]

@suliskh All yours.