Material-ui: Use forwardRef

[lazee]

Hi,

It seems that all component functions are "hidden" when exported through the withStyles method.

• [ x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Given a component App, using component MainDrawer that have a function toggleDrawer. When setting ref attribute on the use of MainDrawer (<MainDrawer ref={(drw) => { this.mainDrawer = drw; }} />) inside the render method in App, I should be able to call this.mainDrawer.toggleDrawer() inside component App.

Current Behavior

When dumping this in component App I see the mainDrawer component that was added. But it doesn't contain the toggleDrawer function for some reason. If i remove the withStyles wrapper from component MainDrawer, then the toggleDrawer function appears in this.mainDrawer and can be executed.

The error I get in console is:

Uncaught TypeError: this.mainDrawer.toggleDrawer is not a function
    at App.doToggle (App.js:27)
    at Object.toggle (App.js:35)
    at onClick (AppBarHeader.js:31)
    at HTMLUnknownElement.callCallback (react-dom.development.js:542)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:581)
    at Object.invokeGuardedCallback (react-dom.development.js:438)
    at Object.invokeGuardedCallbackAndCatchFirstError (react-dom.development.js:452)
    at executeDispatch (react-dom.development.js:836)
    at executeDispatchesInOrder (react-dom.development.js:858)
    at executeDispatchesAndRelease (react-dom.development.js:956)
    at executeDispatchesAndReleaseTopLevel (react-dom.development.js:967)
    at Array.forEach (<anonymous>)
    at forEachAccumulated (react-dom.development.js:935)
    at processEventQueue (react-dom.development.js:1112)
    at runEventQueueInBatch (react-dom.development.js:3607)
    at handleTopLevel (react-dom.development.js:3616)
    at handleTopLevelImpl (react-dom.development.js:3347)
    at batchedUpdates (react-dom.development.js:11082)
    at batchedUpdates (react-dom.development.js:2330)
    at dispatchEvent (react-dom.development.js:3421)

This leads me to expect that something is going on in withStyles, that leads this function to be hidden? Or maybe I have misunderstood it all.

Steps to Reproduce (for bugs)

https://github.com/lazee/material-ui-issue-withstyles

• Clone
• yarn install
• yarn dev
• Click on the menu icon in the page that appears and look in console.

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | latest next |
| React | 16.2.0 |
| browser | Chrome 65 |
| Node | 9.9.0 |

[oliviertassinari]

this.mainDrawer.toggleDrawer()

@lazee Calling imperative methods for controlling elements is a discouraged pattern in React. I would encourage you to use an open like property.

It doesn't contain the toggleDrawer function for some reason.

It's because the withStyles() higher order component is exposing a different component. You have to use the documented innerRef property.

Let's see if we can use forwardRef in the future to improve the story.

[oliviertassinari]

https://reactjs.org/blog/2018/03/29/react-v-16-3.html#forwardref-api

[lazee]

@oliviertassinari Thank you so much for taking the time to answer something that was already documented. I actually looked for it on the site, but must have done a terrible job. My bad.

I did realize that I was using an anti-pattern, but couldn't figure out why it didn't work. With other wrapper from other frameworks I haven't had the same issues. But what you say about "exposing a different component" makes sense. I will rewrite and use property instead.

forwardRef seems like a perfect match for theming. Also looking forward to see what can be done wi th that in the future.

AND: Thank you for Material UI. Love it!

[oliviertassinari]

Let's wait and see how these libraries solve the problem:

• https://github.com/reactjs/react-redux/issues/914
• https://github.com/ReactTraining/react-router/issues/6056

[alimo]

innerRef doesn't solve the problem of forwarding ref through multiple HOCs, but forwardRef does.
Also it would be possible to have both of them available to maintain backward compatibility (I guess).

[oliviertassinari]

@alimo You are right, having following the threads of https://github.com/mui-org/material-ui/issues/10825#issuecomment-383318055. It seems that bringing forwardRef is not trivial.

When you start using forwardRef in a component library, you should treat it as a breaking change and release a new major version of your library.