Skip to content

Commit

Permalink
[ButtonBase] When with wrong component prop
Browse files Browse the repository at this point in the history
  • Loading branch information
oliviertassinari committed Apr 3, 2020
1 parent 861498c commit fbd0db6
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 20 deletions.
23 changes: 11 additions & 12 deletions docs/src/pages/guides/composition/composition.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Material-UI allows you to change the root element that will be rendered via a pr

### How does it work?

The component will render like this:
The custom component will be rendered by Material-UI like this:

```js
return React.createElement(props.component, props)
Expand Down Expand Up @@ -67,9 +67,11 @@ import { Link } from 'react-router-dom';
function ListItemLink(props) {
const { icon, primary, to } = props;

const CustomLink = props => <Link to={to} {...props} />;

return (
<li>
<ListItem button component={props => <Link to={to} {...props} />}>
<ListItem button component={CustomLink}>
<ListItemIcon>{icon}</ListItemIcon>
<ListItemText primary={primary} />
</ListItem>
Expand All @@ -81,15 +83,15 @@ function ListItemLink(props) {
⚠️ However, since we are using an inline function to change the rendered component, React will unmount the link every time `ListItemLink` is rendered. Not only will React update the DOM unnecessarily, the ripple effect of the `ListItem` will also not work correctly.

The solution is simple: **avoid inline functions and pass a static component to the `component` prop** instead.
Let's change the `ListItemLink` to the following:
Let's change the `ListItemLink` component so `CustomLink` always reference the same component:

```jsx
import { Link } from 'react-router-dom';

function ListItemLink(props) {
const { icon, primary, to } = props;

const renderLink = React.useMemo(
const CustomLink = React.useMemo(
() =>
React.forwardRef((linkProps, ref) => (
<Link ref={ref} to={to} {...linkProps} />
Expand All @@ -99,7 +101,7 @@ function ListItemLink(props) {

return (
<li>
<ListItem button component={renderLink}>
<ListItem button component={CustomLink}>
<ListItemIcon>{icon}</ListItemIcon>
<ListItemText primary={primary} />
</ListItem>
Expand All @@ -108,8 +110,6 @@ function ListItemLink(props) {
}
```

`renderLink` will now always reference the same component.

### Caveat with prop forwarding

You can take advantage of the prop forwarding to simplify the code.
Expand Down Expand Up @@ -173,18 +173,17 @@ wrapped component can't hold a ref.
In some instances an additional warning is issued to help with debugging, similar to:
> Invalid prop `component` supplied to `ComponentName`. Expected an element type that can hold a ref.

Only the two most common use cases are covered. For more information see [this section in the official React docs](https://reactjs.org/docs/forwarding-refs.html).

```diff
- const MyButton = props => <div role="button" {...props} />;
+ const MyButton = React.forwardRef((props, ref) => <div role="button" {...props} ref={ref} />);
-const MyButton = props => <div role="button" {...props} />;
+const MyButton = React.forwardRef((props, ref) => <div role="button" {...props} ref={ref} />);
<Button component={MyButton} />;
```

```diff
- const SomeContent = props => <div {...props}>Hello, World!</div>;
+ const SomeContent = React.forwardRef((props, ref) => <div {...props} ref={ref}>Hello, World!</div>);
-const SomeContent = props => <div {...props}>Hello, World!</div>;
+const SomeContent = React.forwardRef((props, ref) => <div {...props} ref={ref}>Hello, World!</div>);
<Tooltip title="Hello, again."><SomeContent /></Tooltip>;
```

Expand Down
33 changes: 26 additions & 7 deletions packages/material-ui/src/ButtonBase/ButtonBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { elementTypeAcceptingRef, refType } from '@material-ui/utils';
import useForkRef from '../utils/useForkRef';
import useEventCallback from '../utils/useEventCallback';
import withStyles from '../styles/withStyles';
import NoSsr from '../NoSsr';
import useIsFocusVisible from '../utils/useIsFocusVisible';
import TouchRipple from './TouchRipple';

Expand Down Expand Up @@ -278,6 +277,28 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
const handleOwnRef = useForkRef(focusVisibleRef, buttonRef);
const handleRef = useForkRef(handleUserRef, handleOwnRef);

const [mountedState, setMountedState] = React.useState(false);

React.useEffect(() => {
setMountedState(true);
}, []);

const enableTouchRipple = mountedState && !disableRipple && !disabled;

if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(() => {
if (enableTouchRipple && !rippleRef.current) {
console.error(
[
'Material-UI: the `component` prop provided to ButtonBase is invalid.',
'Please make sure the children prop is rendered in this custom component.',
].join('\n'),
);
}
}, [enableTouchRipple]);
}

return (
<ComponentProp
className={clsx(
Expand Down Expand Up @@ -307,12 +328,10 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
{...other}
>
{children}
<NoSsr>
{!disableRipple && !disabled ? (
/* TouchRipple is only needed client-side, x2 boost on the server. */
<TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
) : null}
</NoSsr>
{enableTouchRipple ? (
/* TouchRipple is only needed client-side, x2 boost on the server. */
<TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
) : null}
</ComponentProp>
);
});
Expand Down
16 changes: 15 additions & 1 deletion packages/material-ui/src/ButtonBase/ButtonBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ describe('<ButtonBase />', () => {
PropTypes.resetWarningCache();
});

it('warns on invalid `component` prop', () => {
it('warns on invalid `component` prop: ref forward', () => {
// Only run the test on node. On the browser the thrown error is not caught
if (!/jsdom/.test(window.navigator.userAgent)) {
return;
Expand All @@ -936,5 +936,19 @@ describe('<ButtonBase />', () => {
'Invalid prop `component` supplied to `ForwardRef(ButtonBase)`. Expected an element type that can hold a ref',
);
});

it('warns on invalid `component` prop: prop forward', () => {
const Component = React.forwardRef((props, ref) => (
<button type="button" ref={ref} {...props}>
Hello
</button>
));

// cant match the error message here because flakiness with mocha watchmode
render(<ButtonBase component={Component} />);
expect(consoleErrorMock.messages()[0]).to.include(
'Please make sure the children prop is rendered in this custom component.',
);
});
});
});

0 comments on commit fbd0db6

Please sign in to comment.