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

Warning: useLayoutEffect does nothing on the server, because its effect cannot […] #15798

Closed TidyIQ closed 4 years ago

TidyIQ commented 4 years ago

Version 4 of <ButtonBase> now incorporates useLayoutEffect() in the src code here. This wasn't present in version 3.

I understand the need for it however every jest test for a component that has a <Button> within it now results in console warnings like so:

Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-ssr for common fixes.
          in ForwardRef(ButtonBase)
          in WithStyles(ForwardRef(ButtonBase))
          in ForwardRef(Button)
          in WithStyles(ForwardRef(Button)) (at ButtonWithIcon/index.tsx:57)
          in Layout (at ButtonWithIcon/index.tsx:26)
          in ButtonWithIcon (at OAuthButton/index.tsx:58)
          in Layout (at OAuthButton/index.tsx:21)
          in OAuthButton (at OAuthSignUp.tsx:221)
          in div (at OAuthSignUp.tsx:200)
          in Layout (at OAuthSignUp.tsx:138)
          in OAuthSignUp (at OAuthSignUp.unit.test.tsx:60)
          in Context.Provider
          in ThemeProvider (at testProviders.tsx:29)
          in Context.Provider (at store.tsx:36)
          in Provider (at testProviders.tsx:28)
          in Providers (at OAuthSignUp.unit.test.tsx:59)

The tests still pass as it's just a warning, but is there any way to prevent these warning messages from appearing? I literally can't even see all the warnings in my terminal as there are so many.

eps1lon commented 4 years ago

This means your test setup has a DOM while you're using sever-side rendering. I don't think ReactDOM.renderToStaticMarkup has use cases in the browser. I believe you can tell jest to run tests in a specific environment. For your SSR tests you should run in node not jsdom.


Edit by @oliviertassinari

Would it work, in your case with? (from react):


// React currently throws a warning when using useLayoutEffect on the server.
// To get around it, we can conditionally useEffect on the server (no-op) and
// useLayoutEffect in the browser.
const canUseDOM: boolean = !!(
  typeof window !== 'undefined' &&
  typeof window.document !== 'undefined' &&
  typeof window.document.createElement !== 'undefined'
);

const useIsomorphicLayoutEffect = canUseDOM ? useLayoutEffect : useEffect;
ralvs commented 4 years ago

I'm using Next.js and I'm getting the same alert but in Chrome Console when my pages loads. Even though I have updated to the example (https://github.com/mui-org/material-ui/tree/master/examples/nextjs)

eps1lon commented 4 years ago

I'm using Next.js and I'm getting the same alert but in Chrome Console when my pages loads. Even though I have updated to the example (/examples/nextjs@master )

Creating a fresh copy of our example does not log any warnings for me. If you have some custom setup please include a complete reproducible example. The same applies to @TidyIQ. The issue depends on your specific environment. I would guess that you get the same warnings using the latest version of react-redux.

TidyIQ commented 4 years ago

My code is quite large so it will take a bit of time to provide a reproduction, but just to confirm I'm also use Next.js and my tests are being run in jsdom (with jest and react-testing-library), puppeteer (with jsdom-screenshot) and ReactDOM.renderToStaticMarkup (with jest-axe). I haven't yet determined which are causing the issue yet.

eps1lon commented 4 years ago

jsdom (with jest and react-testing-library), puppeteer (with jsdom-screenshot) and ReactDOM.renderToStaticMarkup

That is the issue. You're creating an environment that is quite different from your production environment. On the server you won't have a DOM available.

For tests using ReactDOM.renderToStaticMarkup you should use @jest-environment node

ralvs commented 4 years ago

I'm using Next.js and I'm getting the same alert but in Chrome Console when my pages loads. Even though I have updated to the example (/examples/nextjs@master )

Creating a fresh copy of our example does not log any warnings for me. If you have some custom setup please include a complete reproducible example. The same applies to @TidyIQ. The issue depends on your specific environment. I would guess that you get the same warnings using the latest version of react-redux.

@eps1lon I found the source of issue in my case. Its the use of next/link instead of @material-ui/core/Link

import Link from "next/link"

<Link href="/about">
  <Fab>
    <AddIcon />
  </Fab>
</Link>

<Link href='/contact' color='inherit' className={classes.link}>
    <ListItem button>
      <ListItemIcon>
        <ContactsIcon />
      </ListItemIcon>
      <ListItemText primary='Contatos' />
    </ListItem>
 </Link>

When I click in any of this links, the new page is loaded and the warnings are shown at DevTools

Just changing the import to @material-ui/core/Link resolves the issue.

ralvs commented 4 years ago

FINALLY, I could reproduce the error! 😅 And apparently, the problem is cause when you attach next-with-apollo to the _app.js

If I use the example for Next and Apollo (https://github.com/zeit/next.js/tree/master/examples/with-apollo) the problem do not occur but I lost some good functionalities tha next-with-apollo offers

Steps to do it:

Clone the example of MUI with NextJS (https://github.com/mui-org/material-ui/tree/master/examples/nextjs)

curl https://codeload.github.com/mui-org/material-ui/tar.gz/master | tar -xz --strip=2  material-ui-master/examples/nextjs
cd nextjs
npm i
npm i next-with-apollo apollo-boost graphql

Create the HOC that create an Apollo Client (next-with-apollo.js)

import withApollo from 'next-with-apollo';
import ApolloClient from 'apollo-boost';

export default withApollo(
  ({ headers }) =>
    new ApolloClient({
      uri: 'https://localhost:4000',
      request: operation => {
        operation.setContext({
          fetchOptions: {
            credentials: 'include',
          },
          headers,
        });
      },
    })
);

Attach the HOC on _app.js

import withApollo from '../src/next-with-apollo';
...
export default withApollo(MyApp);

Add some Button to any page! When you click a link to go to that page that have the Button, 11 warnings appear at DevTools

index.js:1 Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-ssr for common fixes.
    in NoSsr
    in button
    in ForwardRef(ButtonBase)
    in WithStyles(ForwardRef(ButtonBase))
    in ForwardRef(Button)
    in WithStyles(ForwardRef(Button)) (at pages/index.js:29)
    in div
    in Styled(MuiBox) (at pages/index.js:25)
    in div
    in ForwardRef(Container)
    in WithStyles(ForwardRef(Container)) (at pages/index.js:24)
    in Index (at _app.js:29)
    in Context.Provider
    in ThemeProvider (at _app.js:26)
    in Container (at _app.js:22)
    in MyApp
    in RenderPromisesProvider
console.<computed> @ index.js:1
warningWithoutStack @ react-dom-server.browser.development.js:104
warning @ react-dom-server.browser.development.js:290
useLayoutEffect @ react-dom-server.browser.development.js:1215
useLayoutEffect @ react.development.js:1482
NoSsr @ NoSsr.js:28
processChild @ react-dom-server.browser.development.js:2887
resolve @ react-dom-server.browser.development.js:2811
render @ react-dom-server.browser.development.js:3201
read @ react-dom-server.browser.development.js:3160
renderToStaticMarkup @ react-dom-server.browser.development.js:3660
process @ react-apollo.esm.js:1038
Promise.then (async)
getMarkupFromTree @ react-apollo.esm.js:1043
getDataFromTree @ react-apollo.esm.js:1009
(anonymous) @ withApollo.js:128
step @ withApollo.js:56
(anonymous) @ withApollo.js:37
fulfilled @ withApollo.js:28
Promise.then (async)
step @ withApollo.js:30
(anonymous) @ withApollo.js:31
push../node_modules/next-with-apollo/lib/withApollo.js.__awaiter @ withApollo.js:27
_a.getInitialProps @ withApollo.js:102
_callee$ @ utils.js:33
tryCatch @ runtime.js:62
invoke @ runtime.js:288
prototype.<computed> @ runtime.js:114
asyncGeneratorStep @ asyncToGenerator.js:5
_next @ asyncToGenerator.js:27
(anonymous) @ asyncToGenerator.js:34
F @ _export.js:36
(anonymous) @ asyncToGenerator.js:23
loadGetInitialProps @ utils.js:33
_callee2$ @ router.js:373
tryCatch @ runtime.js:62
invoke @ runtime.js:288
prototype.<computed> @ runtime.js:114
asyncGeneratorStep @ asyncToGenerator.js:5
_next @ asyncToGenerator.js:27
(anonymous) @ asyncToGenerator.js:34
F @ _export.js:36
(anonymous) @ asyncToGenerator.js:23
getInitialProps @ router.js:383
(anonymous) @ router.js:245
F @ _export.js:36
(anonymous) @ router.js:243
Promise.then (async)
getRouteInfo @ router.js:235
(anonymous) @ router.js:187
F @ _export.js:36
change @ router.js:149
push @ router.js:143
SingletonRouter.<computed> @ router.js:71
Link._this.linkClicked @ link.js:125
onClick @ link.js:193
callCallback @ react-dom.development.js:149
invokeGuardedCallbackDev @ react-dom.development.js:199
invokeGuardedCallback @ react-dom.development.js:256
invokeGuardedCallbackAndCatchFirstError @ react-dom.development.js:270
executeDispatch @ react-dom.development.js:561
executeDispatchesInOrder @ react-dom.development.js:583
executeDispatchesAndRelease @ react-dom.development.js:680
executeDispatchesAndReleaseTopLevel @ react-dom.development.js:688
forEachAccumulated @ react-dom.development.js:662
runEventsInBatch @ react-dom.development.js:816
runExtractedEventsInBatch @ react-dom.development.js:824
handleTopLevel @ react-dom.development.js:4826
batchedUpdates$1 @ react-dom.development.js:20439
batchedUpdates @ react-dom.development.js:2151
dispatchEvent @ react-dom.development.js:4905
(anonymous) @ react-dom.development.js:20490
unstable_runWithPriority @ scheduler.development.js:255
interactiveUpdates$1 @ react-dom.development.js:20489
interactiveUpdates @ react-dom.development.js:2170
dispatchInteractiveEvent @ react-dom.development.js:4882
TidyIQ commented 4 years ago

That must be it. I'm also using Apollo Client.

eps1lon commented 4 years ago

Thanks for the repro. Will check if this still happens using reduxjs/react-redux#1283.

eps1lon commented 4 years ago

@ralvs Repro doesn't start. Could you create a cloneable repository or checkout https://github.com/eps1lon/mui-buttonbase-uselayout-repro and see what's missing? Followed your steps (replaced npm with yarn though). Had to install react-apollo (cannot find module) after that I get [ error ] TypeError: Cannot set property default of #<Object> which has only a getter when running yarn dev

pedela commented 4 years ago

@eps1lon If I'm not mistaken, you ran into the following issue: https://github.com/apollographql/apollo-client/issues/4843

Current workaround seems to be reverting to v0.3.1 or impor the client from apollo-boost/lib/index

Thanks for working on this:-)

eps1lon commented 4 years ago

@PedeLa Thanks for the tip. Can make it work:

$ git clone https://github.com/eps1lon/mui-buttonbase-uselayout-repro.git
$ cd mui-buttonbase-uselayout-repro
$ yarn
$ yarn dev
# goto localhost:3000/about

but I can't reproduce any error. Neither browser nor terminal console log any warnings related to useLayoutEffect.

pedela commented 4 years ago

Strange, I followed your steps exactly and I get: image Edit: Only browser console warnings though, terminal doesn't show any warnings

ralvs commented 4 years ago

I just did the same. Using Chrome Version 74.0.3729.169 (Official Build) (64-bit) On MacOS Mojave v10.14.5

Screen Shot 2019-05-31 at 08 15 25
ralvs commented 4 years ago

I also open a issue on next-with-apollo repo: #60

eps1lon commented 4 years ago

Can anbody reproduce this on a linux distribution? Maybe apollo is mocking some part of the DOM on the server on mac only?

pedela commented 4 years ago

I can reproduce this exact error on an Ubuntu-based distribution.

oliviertassinari commented 4 years ago

I can reproduce on MacOS 10.14.5.

oliviertassinari commented 4 years ago

Ok found it. We can close this issue. You can reproduce the problem without Material-UI:

import React from "react";

const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;

function MadeWithLove() {
  useEnhancedEffect(() => {});

  return (
    <span>
      {"Built with love by the "}
    </span>
  );
}

The problem is that next-with-apollo traverse the tree, by default, on the browser with the server side API:

I have used Apollo extensively for more than a year at onepixel.com. I would be cautious with it, it can be slow at scale, it was our bottleneck n°1 from a performance perspective (graphql + apollo client). I believe you don't need to traverse the React tree with the server side API on the client when switching routes. You can have loaders or a centralized getInitialProps logic instead.

ivawzh commented 4 years ago

@oliviertassinari thanks for finding the root of the bug. However, after I set withApollo(..., { getDataFromTree: 'never' }) and new ApolloClient({ ssrMode: false, ... }), I still see the warning messages. Any hits for me?

oliviertassinari commented 4 years ago

I'm sorry, I can't help more. I'm closing. I could ask on https://github.com/lfades/next-with-apollo/issues/60.

oliviertassinari commented 4 years ago

@ivawzh I have tried it on the reproduction provided by @eps1lon, it solves the problem:

diff --git a/src/next-with-apollo.js b/src/next-with-apollo.js
index b0655aa..ba5d3ee 100644
--- a/src/next-with-apollo.js
+++ b/src/next-with-apollo.js
@@ -13,5 +13,5 @@ export default withApollo(
           headers
         });
       }
-    })
+    }), { getDataFromTree: 'never' }
 );
lifeiscontent commented 4 years ago

@oliviertassinari this issue is not an apollo issue, you should be able to render the tree of UI components both on the server and client.

here's an article illustrating this technique:

https://medium.com/@alexandereardon/uselayouteffect-and-ssr-192986cdcf7a

the issue, and problem to be solved is material-ui should have their own useIsomorphicLayoutEffect so the effects still run on the client and server.

eps1lon commented 4 years ago

@lifeiscontent This is the same technique we are using. The problem is that it doesn't work if you use SSR APIs in the browser.

lifeiscontent commented 4 years ago

@eps1lon I realize now the issue I was seeing was due to using an older version. In the process of upgrading now. 🤞

nsaubi-swi commented 4 years ago

Btw I do have this problem too (jest is spamming with this warning), and I don't use next.js or next-with-apollo. Only react / redux & cie and material-ui (among others, of course).

Do you think I have another lib that is interfering ?

oliviertassinari commented 4 years ago

@nsaubi Is window defined in your test env?

nsaubi-swi commented 4 years ago

const jestConfig = { "testEnvironment": "jsdom", ... @oliviertassinari My jest conf is jsdom, so I bet that yes it is.

And btw we're not using SSR at all. And our code base is still "16.5ish", as I just updated the dependencies and thus we're not using hooks for now.

Edit : Just found this (and I think it's related, will test) : https://github.com/facebook/react/issues/14927#issuecomment-515768079

ndeviant commented 4 years ago

Also got this problem. I have an isomorphic app, and it has "window" object in global scope for server, cause of that server uses useLayoutEffect and I've huge amount of warnings in server console. Checking if window is defined is not enough. Please add checking for "window.document" at least: https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/utils/useEventCallback.js

oliviertassinari commented 4 years ago

I have an isomorphic app, and it has "window" object in global scope for server

Why do you have window in global scope for server? I doubt that isomorphic app is a good answer. Is this for working around a broken dependency? Our NoSsr component could help.

ndeviant commented 4 years ago

Yeah, but anyway that could be useful in other cases. Anyway as this issue occurs under different circumstances, I think simple checking for existence of window is not proper check for defining is this server or client. I think mui shouldn't tell user not to have "window" in global scope. For example 'react-urgent' is working with window.navigator.userAgent, and if we wanna guess client device with on the server, we cannot do it cause there is no window.navigator.userAgent, so setting it would workaround the issue.

oliviertassinari commented 4 years ago

@ndeviant You could use https://material-ui.com/components/use-media-query/, you wouldn't need to "pollute" the global scope with a window object.

It looks like the React context should be used here: https://github.com/medipass/react-ugent/blob/d6c68c367316cedf2598b40909c241fcb6b96c81/src/index.js#L44. This doesn't support concurrent requests. It can only work synchronously, one server-side request after the other.

oliviertassinari commented 4 years ago

I'm happy to have found the existence of ua-parser-js. I will update the ssr documentation of useMediaQuery with it, it can help when client hints are not available.

ndeviant commented 4 years ago

I agree with you, and thanks for your help. But what do you think about this:

I think simple checking for existence of window is not proper check for defining is this server or client. I think mui shouldn't tell user not to have "window" in global scope.

This took me a lot of time to find out that the reason for these warnings is that the material-ui does not correctly recognize is this server or client. And it is based on the presence of the window object

oliviertassinari commented 4 years ago

But what do you think about this:

@ndeviant Do we have a better option?

ndeviant commented 4 years ago

Maybe something like this:

function isServer() {
  return  typeof window === 'undefined' ||
    (typeof global !== 'undefined' &&
      (!window.global || window.global !== global));
}

Edited: added possibility to have global.window.global prop, to avoid any conflicts.

ndeviant commented 4 years ago

Or this: https://github.com/MatthewNPM/is-node/blob/93bd8e2e4801945de06f15008ec4ab9afcf8b45f/index.js#L4

oliviertassinari commented 4 years ago

@ndeviant Sorry, this was an invitation to read the whole thread. @eps1lon has already suggested a great possible approach in https://github.com/mui-org/material-ui/issues/15798#issuecomment-497619267.

Would it work, in your case with? (from react):


// React currently throws a warning when using useLayoutEffect on the server.
// To get around it, we can conditionally useEffect on the server (no-op) and
// useLayoutEffect in the browser.
const canUseDOM: boolean = !!(
  typeof window !== 'undefined' &&
  typeof window.document !== 'undefined' &&
  typeof window.document.createElement !== 'undefined'
);

const useIsomorphicLayoutEffect = canUseDOM ? useLayoutEffect : useEffect;
oliviertassinari commented 4 years ago

@eps1lon Should we reopen and deploy a canUseDOM based solution (from React codebase)? We had 7 different people interacting with the issue in 3 months. It's not a lot, so far, its seems to surface underlying problems in people codebases. I'm happy either way and I would tend to not do anything yet.

Note that it's broader to the layout effect, we run a typeof window !== 'undefined' at a few other places.

ndeviant commented 4 years ago

@oliviertassinari yeah, totally. Thats would be great!

oliviertassinari commented 4 years ago

@ndeviant What progress needs to be done? So far you are the only one, we are aware of, that would benefit from https://github.com/mui-org/material-ui/issues/15798#issuecomment-528311534 and even then, the use case is, as far as I understand it, not valid (it should use the context not a global to propagate the user agent).

ndeviant commented 4 years ago

I described only one case when window on server could be useful. Initial issue creator has 9 "Thumbs up" on this, it seems like people keep running into this issue over and over again, with different circumstances (tests env, not ideal deps, etc.). My actual issue is, I've got the project, written before me, which uses window on server, and go and refactor everything that uses this logic, just to get rid of annoying warning I cannot afford.

To conclude what I was saying previously:

  1. Should 'react-urgent' working with window.navigator.userAgent, instead of context? No, for sure.
  2. Should user have 'window' object defined on server? Probably not.
  3. Is this a 'material-ui' responsibility, to say whether he could have defined 'window' on server? No. Leave it to linters or node itself, especially that warning in console ain't saying anything helpful.

React doesn't takes responsibility to say, that you shouldn't have defined 'window' on server, and checks properly for dom existence. So you think that this isn't a bug, but a feature?

eps1lon commented 4 years ago

So you think that this isn't a bug, but a feature?

It's something we can't control. Unfortunately we only have useLayoutEffect for something that needs to be run during commit. Now react warns if it detects this effect when using the ssr API because it assumes that anything in the effect does affect layout. In our case it does not.

Now we solve this issue by assuming that the SSR api is used on the server and the CSR api is used on the client. But nothing prevents you from using the SSR api on the client as well (or rather in environments with a DOM) which results in these false positives.

We voiced our concern about this warning to the react core team but didn't get any response. At this point we cannot do anything more.

As far as I know there's no way of knowing if a component is rendered with the SSR api or CSR api. If there is then we could solve this issue.

oliviertassinari commented 4 years ago

To the developers coming to this page. If you are affected by this problem, please upvote the issue and provide a reproduction we can look at. The more feedback, the best solution we can find. Thanks.

KVNLS commented 4 years ago

Hello, I reproduced using Enzyme's render function. Here is a sandbox

ccdarvin commented 4 years ago

I have same problema,

index.js:1 Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-ssr for common fixes. in NoSsr in button in ForwardRef(ButtonBase)

Code. `export default props => { return

}`

package: "dependencies": { "@apollo/react-hooks": "^3.1.1", "@material-ui/core": "^4.4.3", "@material-ui/icons": "^4.4.3", "apollo-boost": "^0.4.4", "graphql": "^14.5.8", "next": "^9.0.7", "next-with-apollo": "^4.3.0", "react": "^16.10.1", "react-dom": "^16.10.1" }

oliviertassinari commented 4 years ago

@ccdarvin See https://github.com/lfades/next-with-apollo/issues/60

saltyshiomix commented 4 years ago

@oliviertassinari

Still this error occurs with material-ui@4.5.2.

Internally material-ui uses layoutEffect()?

Or use "useIsomorphicLayoutEffect"?

https://github.com/mui-org/material-ui/issues/15798#issuecomment-528311534

I want to SSR with Material UI.

If you have time, please check out the example which demonstrate this warnings:

https://github.com/saltyshiomix/react-ssr/tree/master/examples/with-jsx-material-ui

Dear,

oliviertassinari commented 4 years ago

@saltyshiomix It seems that the warning comes from https://github.com/saltyshiomix/react-ssr/blob/50895251cb950cfa4ccccf03bff43011b4303e51/packages/core/lib/webpack/material-ui.js#L7.

The usage of the server-side API on the client-side triggers these warnings. @hburrows also reported in #17850 that the CSS is empty is in this case.

So, I would recommend not to two render twice on the client, for performance consideration. You will dodge the issue at the same time. Would that work for you?

saltyshiomix commented 4 years ago

@oliviertassinari

You are perfectly right, and solved the problem!

I misunderstood the APIs, sorry.