Warnings in strict mode

[topheman]

EDIT: For the developers that feel adventurous, they can try:

import {
- createMuiTheme,
+ unstable_createMuiStrictModeTheme as createMuiTheme,
  darken,
} from '@material-ui/core/styles';

It's available since #20523, v4.9.11, our intended version for v5. You can find the documentation at https://material-ui.com/customization/theming/#unstable-createmuistrictmodetheme-options-args-theme.

---

I am playing with Suspense on my project topheman/react-fiber-experiments. I have the following warnings in development:

Warning: Legacy context API has been detected within a strict-mode tree: 

Please update the following components: MuiThemeProvider, WithStyles(AppBar), WithStyles(ButtonBase), WithStyles(Card), WithStyles(CardContent), WithStyles(CssBaseline), WithStyles(Drawer), WithStyles(Footer), WithStyles(Header), WithStyles(HomeContainer), WithStyles(IconButton), WithStyles(MainDrawer), WithStyles(MainLayout), WithStyles(Modal), WithStyles(Paper), WithStyles(SvgIcon), WithStyles(Toolbar), WithStyles(Typography)

Learn more about this warning here:
https://fb.me/react-strict-mode-warnings

Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of ButtonBase which is inside StrictMode. Instead, add a ref directly to the element you want to reference.

    in button (created by ButtonBase)
    in ButtonBase (created by WithStyles(ButtonBase))
    in WithStyles(ButtonBase) (created by IconButton)
    in IconButton (created by WithStyles(IconButton))
    in WithStyles(IconButton) (at Header.js:76)
    in div (created by Toolbar)
    in Toolbar (created by WithStyles(Toolbar))
    in WithStyles(Toolbar) (at Header.js:75)
    in header (created by Paper)
    in Paper (created by WithStyles(Paper))
    in WithStyles(Paper) (created by AppBar)
    in AppBar (created by WithStyles(AppBar))
    in WithStyles(AppBar) (at Header.js:74)

Learn more about using refs safely here:
https://fb.me/react-strict-mode-find-node

The Suspense part (asynchronous fetching, placeholders ...) works ok out of strict mode - check the demo.

Though, the async render part (time slicing) may need strict mode (check the link bellow). This is something to dig into now, people will soon be trying alphas of Suspense and a new major version of react will come in a few months.

Resource to read

Related: Migrate from findDOMNode to refs

Your Environment

Tech	Version
Material-UI	v3.1.2
React	v16.6.0 - custom build from this commit
Browser	Chrome

More infos:

• https://react-fiber-experiments.surge.sh/about
• https://github.com/topheman/react-fiber-experiments

[eps1lon]

We can't get rid of findDOMNode in many cases as I pointed out previously: #13221 (comment)

Is there currently a way to check from a component if it is in StrictMode? That way we could avoid findDOMNode and somehow communicate to users that their components should properly forward their refs.

The context API rewrite should hopefully not cause any breakage but I'm not very familiar with our theming approach.

[topheman]

Is there currently a way to check from a component if it is in StrictMode? That way we could avoid findDOMNode and somehow communicate to users that their components should properly forward their refs.

It seems that you need to have access to the current fiber in process : https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberBeginWork.js#L946-L948 though, this will be a common need for library authors.

[eps1lon]

I think that line is about context.

So I looked at the code of findDOMNode and did some testing and it turns out that React does not warn if you pass a dom element element type which is perfect for us. We can still use findDOMNode but test our tree in strict mode to be ready for Supsense StrictMode.

Example: https://codesandbox.io/s/qko6n6vzjj

[topheman]

Just to be clear, it currently works with Suspense (this is the Async rendering part which might pose problem - like Time Slicing).

Your example is perfect 👍- at least we know that there is an escape hatch anyway.

However, this means, that each time the user passes a component prop, he/she will have to create a new one with React.forwardRef before using it.

Example:

<Toolbar>
  <Button
    component={Link}
    to="/"
    title="Home"
  />
  <IconButton
    component={Link}
    to="/foo"
    title="Foo"
  />
  <Typography
    component={Link}
    to="/"
    title="Bar"
  >
    Bar
  </Typography>
</Toolbar>

[eps1lon]

@topheman Might be a good idea to add a note to the documentation where people can pass their own component and we use findDOMNode on their refs and require that those components forward their refs properly in the next major or so.

We could also issue deprecation warnings if findDOMNode(ref) !== ref but this won't provide useful error stacks unless we actually throw. I wish the warning package would include the component stack.

Also the react docs recommend using forwardRef with a new major because it could be considered breaking behavior (with which I agree) but we have to be careful not to introduce to many breaking changes in our next major because people might not be able to upgrade.

[gaearon]

React does not warn if you pass a dom element which is perfect for us.

If you pass a DOM element, why do you need findDOMNode at all? It will just give you that element back.

[eps1lon]

React does not warn if you pass a dom element which is perfect for us.

If you pass a DOM element, why do you need findDOMNode at all? It will just give you that element back.

@gaearon
We don't pass the DOM elements but the user. We interface the as prop in Button so that it accepts anything that can hold refs.

interface Props {
  as: React.ComponentClass | React.ForwardRefExoticComponent | string;
}
function Button({ as: Component }: Props) {
  // users in strict mode would be required to forward refs to the DOM node
  // while users in "loose" mode can simply pass in anything that can hold refs
  return <Component ref={ref => findDOMNode(ref)!.focus()} />
}

// valid
<Button as="div" />
<Button as={SomeClassComponent} />
<Button as={SomeComponentThatForwardsRefs} />
<Button as={ReactRouterLink} />
// invalid but was valid via findDOMNode(this) in the Button implemented as a class component
<Button as={() => <ReactRouterLink to="/somewhere" />} />

[eps1lon]

Tracking progress here.
Test plan:

• run unit test in React.StrictMode: https://github.com/eps1lon/material-ui/tree/test/strict-mode
• enable StrictMode in docs (/pages/_app.js)

[oliviertassinari]

An update on our current progress.

We have solved most the issue in Material-UI. We dependent on reactjs/react-transition-group#429 to be 100% compliant.

[sibelius]

since with version mui has solved most strict issues?

[oliviertassinari]

@sibelius v4.5.1 is a safe bet 😁.

[EddyVinck]

@topheman Do you know if this is still an issue in v5 beta?

[oliviertassinari]

Looking at the results of https://github.com/mui-org/material-ui/search?q=StrictModeViolation.

It seems that we are done. The instances where it fails are related to the soon-to-be deprecated @material-ui/styles package. @eps1lon Is it accurate? Is there anything left to do before we can close?

[eps1lon]

Public documentation that these methods are neither StrictMode nor React 18 compatible is missing. Then we can close. For StrictMode issues in React 18 I would start a new issue.

[eps1lon]

With the release of v5 (release candidate next week) all StrictMode related issues for React 17 (and current 18) will be fixed. For new StrictMode issues in React 18 we'll want to open a new issue (if they come up).

Happy to close this issue before its 3 year anniversary and thank you for your patience.