[RFC] Refs in v4

[eps1lon]

refs in v3

1. refs are not included in the API documentation on material-ui.com
2. type declarations forbid ref e.g. <Button ref={handleRef} /> but allow innerRef for every component
   • The latter is wrong in some cases. E.g. The inner components of Paper or Backdrop are function components and can therefore not hold refs. It will trigger a runtime warning.
3. withStyles from @material-ui/core does not forward refs from ref but innerRef.
   • named props for refs can be problematic if multiple HOCs are used. E.g. Using styled-components to style material-ui components will prevent innerRef from being passed to the inner material-ui component.

Proposal

explicitly document ref

Couple of alternatives:

1. We could start by collecting what components allow this at the moment (basically every component wrapped in withStyles). This will break in v4 if [styles] Improve ref forwarding #13676 is accepted
2. Disallow ref on all of our components. If the DOM node is required RootRef should be used.

The type declarations follow from the documentation.

ref in v4

With #13676 and the replacement of @material-ui/core/styles with @material-ui/styles as our internal styling solution <Button ref /> would return the ref to the inner component by default. In it's current state some components would loose the ability to hold ref (e.g. AppBar).

There is an argument to be made that ref inherently cause all sorts of problems in general. However I would argue that all of our components return some html element. Interfacing our components so that they only return a ref to an abstract HTMLElement should therefore cause no problems in the future. It doesn't bind us to any specific implementation (other than a html element). This allows e.g. layout computations if necessary or imperative focus which is what we use internally.

I'm in favor of enabling refs on all of our existing components. It allows for a simpler API where one doesn't have to look at the API docs for every single component and check if it allows ref (and then be annoyed because we didn't have your use case in mind). (Although TS users can be lazy and don't have to check API docs).

Summary

1. Add explicit documentation
2. Fix typescript declarations
3. One of
   1. Disallow ref on all components i.e. use ref at your own risk. Prefer RootRef if you need the DOM node
      • RootRef will trigger findDOMNode deprecation warnings
   2. Don't change ref behavior for inner components. ref behavior on actual component will change though with [styles] Improve ref forwarding #13676.
      • users are stuck with findDOMNode
   3. Allow ref on all components. Interface it as a abstract HTMLElement
      • strict and concurrent mode viable for all current use cases

Confirmed

• add explicit documentation
• fix typescript declarations
• add migration guide [docs] Document ref forwarding (requirements) #15298
• add prop-type if we require refs in the component
• forward refs for every component (started in [core] forward innerRef for certain components #14536)

Currently missing (strike through means not applicable, some might already work but are not picked up by our docs generator)

• AppBar.js
• Backdrop.js
• Avatar.js
• BottomNavigation.js
• BottomNavigationAction.js
• Breadcrumbs.js
• [ ] TouchRipple.js (need imperative handle)
• Card.js
• CardActionArea.js
• CardActions.js
• CardContent.js
• CardHeader.js
• CardMedia.js
• Checkbox.js
• CircularProgress.js
• [ ] ClickAwayListener.js no host component rendered
• Container.js
• [ ] CssBaseline.js no host component rendered
• Dialog.js
• DialogActions.js
• DialogContent.js
• DialogContentText.js
• DialogTitle.js
• Divider.js
• ExpansionPanel.js
• ExpansionPanelActions.js
• ExpansionPanelDetails.js
• ExpansionPanelSummary.js
• Fab.js
• [ ] Fade.js no host component rendered
• FilledInput.js
• FormControl.js
• FormControlLabel.js
• FormGroup.js
• FormHelperText.js
• FormLabel.js
• Grid.js
• GridList.js
• GridListTile.js
• GridListTileBar.js
• [ ] Grow.js no host component rendered
• [ ] Hidden.js no host component rendered
• Icon.js
• IconButton.js
• Input.js
• InputAdornment.js
• InputLabel.js
• Link.js
• List.js
• ListItem.js
• [ ] ListItemAvatar.js no host component rendered
• ListItemIcon.js
• ListItemSecondaryAction.js
• ListItemText.js
• ListSubheader.js
• Menu.js
• MenuItem.js
• MenuList.js
• MobileStepper.js
• NativeSelect.js
• [ ] NoSsr.js no host component rendered
• OutlinedInput.js
• Paper.js
• Popper.js
• [ ] Portal.js no host component rendered
• Radio.js
• RadioGroup.js
• [ ] RootRef.js no host component rendered
• Select.js
• SnackbarContent.js
• Step.js
• StepButton.js
• StepConnector.js
• StepContent.js
• StepIcon.js
• StepLabel.js
• Stepper.js
• SvgIcon.js
• Switch.js
• Tab.js
• Table.js
• TableBody.js
• TableCell.js
• TableFooter.js
• TableHead.js
• TableRow.js
• TableSortLabel.js
• TextField.js
• Toolbar.js
• Typography.js
• [ ] Zoom.js no host component rendered
• Badge.js
• Button.js
• Collapse.js
• Drawer.js
• LinearProgress.js
• [ ] Slide.js no host component rendered
• Snackbar.js
• TablePagination.js
• [ ] Tooltip.js Renders children as root, PopperProps available
• ButtonBase.js
• Chip.js
• InputBase.js
• Modal.js
• SwipeableDrawer.js
• Tabs.js
• Popover.js
• [ ] SpeedDialIcon.js lab has low priority
• [ ] Slider.js lab has low priority
• [ ] SpeedDialAction.js lab has low priority
• [ ] SpeedDial.js lab has low priority
• [ ] ToggleButton.js lab has low priority
• [ ] ToggleButtonGroup.js lab has low priority

Resources

• Refs and the DOM
• RootRef
• issue: Warnings in strict mode #13394
• issue: Use forwardRef #10825

/cc @mui-org/core-contributors

[TrySound]

Allow ref on all components. RootRef should be already deprecated since it's warned in strict mode. When fragment refs will come we can't know.

[oliviertassinari]

Allow ref on all components. This is the best solution from a DX perspective. I think that we should aim for it, as long as, the UX implications are acceptable. For instance, does it requires us to increase the bundle size? Does is slow the components down? I don't have the answer to these questions. I only know that using forwardRef on a class component is a "nightmare" but we can build tools around the problem. What if we move all our components to functions leveraging hooks?

[TrySound]

That would awesome. Will force users to upgrade react though.

We could also use hooks to get rid from cloneElement which failed for me a few times when I need to wrap MenuItem.

[eps1lon]

For instance, does it requires us to increase the bundle size?

Yes, because we have to use React.forwardRef. However the increased bundle size should be very small. gzip might result in a O(1) increase instead of O(n) where n is the number of components affected.

Does is slow the components down?

The more interesting question would be "by how much" since any added behavior slows it down. I remember a discussion about this concerning forwardRef in react-redux and the gist was basically that while it was slower it wasn't because of forwardRef itself but because any additional component is slower. However this is a micro-benchmark fallacy. Sure you slow down one specific code path down by e.g. 100% but if that codepath only takes up .001% of your hole app then the micro benchmark is not that interesting.

I only know that using forwardRef on a class component is a "nightmare" but we can build tools around the problem.

I had the same issue originally but I'm comfortable with it now. The fact that ref is not treated like other props is what makes this hard. There is an initial learning curve where you have to switch from "a component has props" and "a component has props and a ref which is not visible in the syntax". I don't think this pattern is that complicated: Declare a forwardRef on the class component, use forwardRef((props, ref) => <ClassComponent forwardRef={ref} {...props} />).

What if we move all our components to functions leveraging hooks?

What do hooks solve WRT to ref? Function components would result in "no ref allowed".

[TrySound]

@eps1lon

export const Menu = React.forwardRef((props, ref) => {
  const [open, setOpen] = React.useState(false);
  return (
    <div ref={ref} onClick={() => setOpen(v => !v)}>{open ? 'open' : 'closed'}</div>
  );
});

[eps1lon]

@eps1lon

export const Menu = React.forwardRef((props, ref) => {
  const [open, setOpen] = React.useState(false);
  return (
    <div ref={ref} onClick={() => setOpen(v => !v)}>{open ? 'open' : 'closed'}</div>
  );
});

What do hooks solve here with regards to ref?

[TrySound]

You don't need additional component wrapper like with classes.

[eps1lon]

You don't need additional component wrapper like with classes.

The usage of state in your example is arbitrary. You picked one feature of classes this.setState and ported this to hooks. However, you did non show me how hooks solved any issue related to ref.

Try not to think about any specific implementation detail here. The original point

What if we move all our components to functions leveraging hooks?

did not adress the concern of this issue. We already have function components that do not use hooks. What could you achieve with hooks in function components that solves any issue with refs?

For example using <AppBar innerRef /> will result in a runtime warning because the inner component is a function component. In v3 you could however do <AppBar ref /> because the inner component is wrapped in withStyles which wraps the inner component in a class component. If we forward ref in withStyles then users don't have the possibility to get the DOM node without using RootRef. That is what I wanted to adress: Do we forward ref on the inner component too or do we not allow ref on our components (or do we revert ref forwarding in withStyles and force users to use findDOMNode).

[TrySound]

My point was that hooks works inside of forwardRef function. So performance case you mentioned before is solved.

I believe you won't use withStyles with hooks. You already have better tools. So infinite components wrappers will gone. This should also increase performance.

You will not end up with runtime warning. That's the point of forwardRef, isn't it?

[eps1lon]

My point was that hooks works inside of forwardRef function. So performance case you mentioned before is solved.

Probably if we actually port class components to function components. My statement was only made in the context of "given a component: what perf implication has forwardRef" which especially applies to already existing function components. But I think you implied that we wouldn't wrap those in withStyles but rather use the hooks implementation so the perf might even out. Again: Probably but let's discuss one issue at a time. We didn't officially determine if we want to bump the peer dependency on react to 16.8.

I believe you won't use withStyles with hooks. You already have better tools. So infinite components wrappers will gone. This should also increase performance.

Maybe. I haven't seen any benchmarks comparing class components with function components + hooks though.

You will not end up with runtime warning. That's the point of forwardRef, isn't it?

Exactly. This is the discussion we have: use forwardRef or not.

[oliviertassinari]

I have looked into the performance implication. It's not significant. I think that we can ignore the problem. I have tried the following:

function NakedButton(props) {
  return <button type="button" {...props} />;
}

class HocButton extends React.Component {
  state = {};

  render() {
    return <NakedButton {...this.props} />;
  }
}

const ForwardRef = React.forwardRef((props, ref) => <button ref={ref} type="button" {...props} />);

suite
  .add('NakedButton', () => {
    ReactDOMServer.renderToString(<NakedButton>Material-UI</NakedButton>);
  })
  .add('ForwardRef', () => {
    ReactDOMServer.renderToString(<ForwardRef>Material-UI</ForwardRef>);
  })
  .add('HocButton', () => {
    ReactDOMServer.renderToString(<HocButton>Material-UI</HocButton>);
  })
  .on('cycle', event => {
    console.log(String(event.target));
  })
  .run();

It outpus the following:

NakedButton x 738,357 ops/sec ±3.19% (187 runs sampled)
ForwardRef x 674,264 ops/sec ±1.22% (188 runs sampled)
HocButton x 423,407 ops/sec ±2.22% (181 runs sampled)

[eps1lon]

I have looked into the performance implication. It's not significant.

Good to know.

[oliviertassinari]

Do we have any concern left to address?

[eps1lon]

Do we have any concern left to address?

I don't think so. My primary concern was to consolidate all the concerns (and gather my own thoughts). If we implement this then #13394 should be resolved completely. Just looks better on paper to be "strict mode compliant".

[oliviertassinari]

Alright, let's execute it then :)

[oliviertassinari]

💥