mui / material-ui

Materialย UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Warnings in strict mode #13394

Closed topheman closed 2 years ago

topheman commented 5 years ago

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:

eps1lon commented 5 years ago

We can't get rid of findDOMNode in many cases as I pointed out previously: https://github.com/mui-org/material-ui/issues/13221#issuecomment-429451615

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Tracking progress here. Test plan:

oliviertassinari commented 4 years ago

An update on our current progress.

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

sibelius commented 4 years ago

since with version mui has solved most strict issues?

oliviertassinari commented 4 years ago

@sibelius v4.5.1 is a safe bet ๐Ÿ˜.

chybisov commented 4 years ago

An update on our current progress.

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

Maybe we should consider move on our own way without slow developing dependencies? ๐Ÿ˜

eps1lon commented 4 years ago

@oliviertassinari I'm a little bit confused ๐Ÿ˜ why do we have working solution but using buggy one?

The actual react-transition-group is not "buggy". It only issues warnings in StrictMode.

We can't use @material-ui/react-transition-group because some of our components that require a ref to their child also allow plain function components. For those we do need findDOMNode.

It was an oversight on my part when working on this for v4. You should be able to alias react-transition-group to @material-ui/react-transition-group in order to remove findDOMNode warnings. However, this library is still not concurrent nor StrictMode compatible with this alias due to our styling solution. I'm not sure if styled-components is StrictMode compatible

sibelius commented 4 years ago

can we have a checklist of items that still need to be fixed?

eps1lon commented 4 years ago

can we have a checklist of items that still need to be fixed?

Grep the codebase for StrictModeViolation. Should be only related to react-transition-group or enzyme tests or makeStyles usage.

a-tonchev commented 4 years ago

Hello guys, since the new CRA is in strict mode, I face the same problem with opening a Menu or opening the Drawer.

What do you think guys, can we expect solution soon, or we should just ignore the warning for now?

Thanks

eps1lon commented 4 years ago

This will be solved in v5. We can't fix it without a breaking change. I'll put together a gist how to fix this in your app by using the theme.

eps1lon commented 4 years ago

Hey @eps1lon, when you have the link for the gist you mentioned above, please let us know. :)

Thanks for the reminder. I have this working partially on some side-projects but I need to be careful with communicating it since it relies on more internals than I'd like. Considering this got quite a bit of upvotes I'll look into this more considering Easter won't be as busy as usual.

oliviertassinari commented 4 years ago

For the developers that feel adventurous, they can try:

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

(available since #20523, v4.9.11, our intended version for v5)

NMinhNguyen commented 4 years ago

makeStyles usage.

Do we know what it's going to take to make makeStyles StrictMode compatible? Is it the use of useSynchronousEffect? I'm aware of the plan to move to Styled Components, but I'm curious how/if they solved this problem.

eps1lon commented 4 years ago

There are multiple issues that make makeStyles not concurrent safe. Being able to use a library that is dedicated to this problem space should free up some resources.

NMinhNguyen commented 4 years ago

There are multiple issues that make makeStyles not concurrent safe.

If it's not too much trouble, would you be able to spell them out? For the benefit of those who may want to contribute a solution, or even to understand the problem space better. But I agree with the core team wanting to spend resources on using an alternative library instead.

eps1lon commented 4 years ago

We are not using createUseStyles but a custom implementation that is very similar. Since the issues are the same I suspect fixes in one could be ported to another.

If you want to deep-dive you should apply

-    mount = createMount({ strict: undefined });
+    mount = createMount({ strict: true });

to https://github.com/mui-org/material-ui/blob/27471b4564eb40ff769352d73a29938d25804e45/packages/material-ui-styles/src/makeStyles/makeStyles.test.js#L42

and make them pass.

Though you might want to flip to false first and make those pass. An explicit false just adds wrapper component so that you're forced to write tests that assert the same component structure as with StrictMode.

jeromeSH26 commented 4 years ago

although I have put the drawer inside the forwardRef, I still have the issue. And the weird part is that when adding this forwardRef, the transition disapeared.

const FwdDrawer = React.forwardRef<
        RefObject<HTMLSpanElement>,
        IDrawerInnerType
    >((props, ref) => <SwipeableDrawer ref={ref} {...props} />);

    return (
        <FwdDrawer
            open={props.open}
            onOpen={() => setOpenedDrawer(true)}
            onClose={() => setOpenedDrawer(false)}
            ModalProps={{
                keepMounted: true,
            }}
            anchor="left"
        >
            <div className={classes.drawerHeader}>
                <IconButton
                    onClick={() => setOpenedDrawer(false)}
                    style={{ color: colorsPalette.white }}
                >
                    {theme.direction === "ltr" ? (
                        <ChevronLeftIcon />
                    ) : (
                        <ChevronRightIcon />
                    )}
                </IconButton>
            </div>
            <Divider />
            {menuList}
        </FwdDrawer>
    );

image

luminaxster commented 4 years ago

@oliviertassinari unstable_createMuiStrictModeTheme is so cool! Those warnings were driving me nuts. May I update the theming docs?. A quick paragraph mentioning strict mode theming is available but unstable.

antonlashan commented 4 years ago

May be disable StrictMode

replace

ReactDOM.render(
  <React.StrictMode>
    <App />
  </React.StrictMode>,
  document.getElementById('root')
);

to

ReactDOM.render(
  <React.Fragment>
    <App />
  </React.Fragment>,
  document.getElementById('root')
);

https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

pReya commented 4 years ago

@26rahulsingh Not sure what you mean, the export for this function is right here: https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/styles/index.js

I think you might be interpreting your linter warning wrong: There are no type definitions, yet. So in a Typescript project, the linter will show an error that the import can't be found.

eps1lon commented 4 years ago

Could we update components?

I'm trying to get this to work but no guarantees. It's somewhat difficult since we forgot to put an important restriction on Snackbar during the v4 development.

eps1lon commented 3 years ago

Starting with @material-ui/core@^4.10 you can use unstable_createMuiStrictModeTheme which should fix all the findDOMNode related warnings. Please read the restrictions carefully.

a-tonchev commented 3 years ago

Starting with @material-ui/core@^4.10 you can use unstable_createMuiStrictModeTheme which should fix all the findDOMNode related warnings. Please read the restrictions carefully.

It says that we should not use this in production ... :(

eps1lon commented 3 years ago

It says that we should not use this in production ... :(

It's a precaution since we can't be sure whether this works for your app or not. Since it only fixes dev-only warnings you shouldn't take the risk.

MariuzM commented 3 years ago

Could i get clarification? There is nothing much we can do apart to remove StrictMode in React project, we just have to wait until Material UI updates it's library?

I did notice we can use unstable_createMuiStrictModeTheme but in my case I'm using Menu button, so this won't work?

import IconButton from '@material-ui/core/IconButton'
import Menu from '@material-ui/core/Menu'
import MenuItem from '@material-ui/core/MenuItem'
import MoreVertIcon from '@material-ui/icons/MoreVert'
oliviertassinari commented 3 years ago

unstable_createMuiStrictModeTheme impacts the Menu and many other components.

raviSussol commented 3 years ago

Hi @oliviertassinari / @eps1lon - has this issue been fixed?? I'm still getting a findDOMNode warning when opening Snackbar from button click in strict mode. I'm using following React and material ui versions:

And here's a sample code of what I did so far. I tried to follow as an example (mentioned by @eps1lon) from here but still getting that Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of Transition which is inside StrictMode.

Do you guys have an idea of getting around of this warning in strict mode except disabling the strict mode itself? Thanks in advance.

raviSussol commented 3 years ago

Thanks @jsonnikson - using unstable_createMuiStrictModeTheme wipes the findDOMNode warning out for now. Hopefully, v5 gets released soon and has a stable solution for this warning.

agzamovr commented 3 years ago

react-transition-group fixed this issue in version 4.4.0. Will this version be included in version 4 of Material UI or it will be in version 5?

lukas-bunat commented 3 years ago

Also confirming that #13394 is fixable just by upgrading to v5. Thanks guys for the effort. ๐Ÿ‘

afroguy16 commented 3 years ago

Also confirming that #13394 is fixable just by upgrading to v5. Thanks guys for the effort. ๐Ÿ‘

Is V5 out?

lukas-bunat commented 3 years ago

Also confirming that #13394 is fixable just by upgrading to v5. Thanks guys for the effort. ๐Ÿ‘

Is V5 out?

I checked my commits and found out that I updated react-tooltip from 4.2.7 to 4.2.8. I don't why I wrote v5, sorry about that. Needless to say, my issue was fixable purely by updating my dependencies by running:

npx npm-upgrade npm i

jfeigel commented 3 years ago

@afroguy16 V5 is still in alpha but if you install @material-ui/core@next the warning goes away.

ZachMurray commented 3 years ago

@afroguy16 V5 is still in alpha but if you install @material-ui/core@next the warning goes away.

Is there any repercussions from doing so? Is the alpha safe enough to use? I know each team has their own definition of what's alpha worthy.

oliviertassinari commented 3 years ago

@ZachMurray alpha as breaking changes potentially every week.

oliviertassinari commented 3 years ago

@eps1lon From what I understand, we can close this issue now (v5.0.0-alpha.7) and only keep #18018 open. Do you confirm?

eps1lon commented 3 years ago

This is an umbrella issue for all issues related to StrictMode. It shouldn't be closed if the underlying issues remain.

Rondip commented 3 years ago

Still having this issue

jeromeSH26 commented 3 years ago

using v5, no more this issue

jnrbo commented 3 years ago

Anyway to solve it without using v5?

oliviertassinari commented 3 years ago

Anyway to solve it without using v5?

@jnrbo Yes, https://github.com/mui-org/material-ui/issues/13394#issuecomment-619266831, or remove the strict mode wrapper.

oliviertassinari commented 3 years ago

In theory, #24405 solves #20708, the last standing issue with have with StrictMode.

eps1lon commented 3 years ago

In theory, #24405 solves #20708, the last standing issue with have with StrictMode.

That's unclear at this moment. We still have plenty of components not tested in StrictMode.