mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.36k stars 32.13k forks source link

Un stable rendering of production build for material UI example for nextJS #9640

Closed ghost closed 6 years ago

ghost commented 6 years ago

Expected Behavior

When page is reloaded, the render should be stable. It is correct in the development build, i.e when I do npm run dev

development

Current Behavior

When page is loaded, the render is unstable. It happens when I run npm run build and npm run start

production

Steps to Reproduce (for bugs)

Following the instructions at https://github.com/zeit/next.js/tree/canary/examples/with-material-ui-next

  1. curl https://codeload.github.com/callemall/material-ui/tar.gz/v1-beta | tar -xz --strip=2 material-ui-1-beta/examples/nextjs
  2. cd nextjs
  3. npm run build
  4. npm run start

Context

Your Environment

Tech Version
Material-UI 1.0.0-beta.25
React 16.2.0
browser Chrome 63.0.3239 on MacBook
Next 4.2.1
oliviertassinari commented 6 years ago

I can't reproduce with the following tests:

curl https://codeload.github.com/mui-org/material-ui/tar.gz/v1-beta | tar -xz --strip=2  material-ui-1-beta/examples/nextjs
cd nextjs
yarn
yarn build
yarn start

You linked callemall/material-ui/tar.gz in the issue description, you might be relying on an out of date version.

ghost commented 6 years ago

I will create a pull request on examples section on nextjs repo to change that from callemall to mui-org. However, the problem is still existing for me even after using new path. Is there any way I can provide some more data to you in re-creating the issue? I am working on Node version: v8.9.3. Thanks!

oliviertassinari commented 6 years ago

I will create a pull request on examples section on nextjs repo to change that from callemall to mui-org.

@credifiable Oh yes please!

Maybe it's a npm cache issue. Have you tried with yarn?

ghost commented 6 years ago

Maybe it's a npm cache issue. Have you tried with yarn?

Yep. Tried with yarn (v1.3.2) too. Since, it is not breaking for you, will try from a different machine also, just to be double sure it's not breaking due to some mysterious powers local to my setup. Thanks!

oliviertassinari commented 6 years ago

@credifiable Let us know. We need to get to the bottom of this issue. It's confusing.

JeromeFitz commented 6 years ago

I cannot duplicate this either. I'd add on, that with SSR the initial render should be stable as well.

Perhaps the node version may have something to do with this? I will attempt to update an OSS deployment on 8.9.3 when that is back up.

Thank you for updating to point to the correct repo in your PR!

ghost commented 6 years ago

Ok. I tried on a different machine, and it was working as expected. To demonstrate you guys what issue I am facing on my machine, I have made a video. I am more than happy to provide you guys any further details to go into bottom of this issue. Thanks a lot for your time.

ghost commented 6 years ago

@oliviertassinari It would be helpful to reopen this issue for some time to give it a bit more exposure. Either I am doing something incredibly incredibly stupid, or this is a genuine issue.

oliviertassinari commented 6 years ago

@credifiable I still can't reproduce with the same dependencies as you have and https://github.com/mui-org/material-ui/issues/9640#issuecomment-354283522:

Do you have any warnings in the console? What's the ouput client side and server looks like?

JeromeFitz commented 6 years ago

I am unable to duplicate this with node@8.9.3 either, unfortunately:

image

oliviertassinari commented 6 years ago

@credifiable Sorry, but you seem to be the only one affected. We can't do anything without being able to reproduce. Keeping the issue open won't help with the exposure, most people use the search of GitHub to find old issues they are suffering. Maybe someone will face the same issue and dig into the rabbit hole.

@JeromeFitz Thank you for the live environment.

ghost commented 6 years ago

Do you have any warnings in the console?

No. No warning or error.

And I understand. Even I couldn't reproduce it using another machine that I had. I will try and and figure out whatever is wrong with my setup, and reply on this thread. Thanks a lot for your time.

ghost commented 6 years ago

Hey @oliviertassinari , I have decided to dedicate today in investigating this issue. Though, I am pretty new to the universe of JavaScript, I will keep on sharing my findings with you, so that if you hit any weird pattern, you can point me in right direction.

I define Error setup as my machine, on which I found the issue. Success setup is another machine I used to take appropriate diffs in order to demonstrate my findings. parsed elements is the html which is shown in the elements tab of developer tools in chrome. I have copied the html from that using edit html feature.

curl diff between Error setup and Success setup: https://www.diffchecker.com/BfPi37Lm

parsed elements diff between Error setup and Success setup: https://www.diffchecker.com/WXvviUdE

npm list diff between Error setup and Success setup: https://www.diffchecker.com/lVEhKuT5

I am now reading how HTML code is generated using nextjs framework, to understand why there is a curl difference between two setups. Thanks!

ghost commented 6 years ago

I tried to dive into the source code of Next, but this is really beyond my skillset, in the given timeframe. I have no option left, but to erase my drive, and re-install MacOS on it. That is the fastest workaround for me, if I want to use my current system.

oliviertassinari commented 6 years ago

I have no option left, but to erase my drive

@credifiable I'm sorry you have to go down this path.

mtford90 commented 6 years ago

I'm also experiencing this unfortunately. I've just encountered it after introducing material-ui and am currently debugging but just wanted to register myself as being affected ;)

I'll try & provide a repo with a reproduction of the issue.

I see this issue in both development & production builds with errors like the following available in the dev build:

main.js:773 Warning: Prop `className` did not match. Server: "MuiTypography-root-85 MuiTypography-headline-90" Client: "MuiTypography-root-31 MuiTypography-headline-36"

Warning: Prop `style` did not match. Server: "box-sizing:border-box;min-height:1px;position:relative;padding-left:15px;padding-right:15px;width:100%;overflow:hidden;flex-basis:50%;flex-grow:0;flex-shrink:0;max-width:50%;margin-left:0%;right:auto;left:auto" Client: "box-sizing:border-box;min-height:1px;position:relative;padding-left:15px;padding-right:15px;width:100%;overflow:hidden;flex-basis:100%;flex-grow:0;flex-shrink:0;max-width:100%;margin-left:0%;right:auto;left:auto"
bendiy commented 6 years ago

I've had this error several months ago and just ran into it again today. Using the next.js example, it happens if withRoot() is called on a component that is not a page or possibly if withRoot() is not called on anything in the ./pages directory.

https://github.com/mui-org/material-ui/blob/ac58d7b2c070cbd3d3df835063f56feed4944147/examples/nextjs/pages/index.js#L73

ghost commented 6 years ago

@bendiy - But shouldn't this behaviour be consistent for a given setup? I mean, with same version of npm, node, and packages? On a separate note - Are you suggesting to change the sequence of application of HOCs(Higher Level Components) in this case? Example: withStyles(styles)(withRoot(Index))? Haven't tested it though!

bendiy commented 6 years ago

@credifiable I know export default withRoot(withStyles(styles)(Index)); works on the index.js page. If you switch it to something like this, it can cause the error: export default withStyles(styles)(withRoot(Index)); or export default withStyles(styles)(Index);.

I'll try to make an example tomorrow.

I've seen it twice now, months apart, and it has something to do with the call order (or lack of calling) withRoot() and withStyles(styles). If you get them right, the rendering issue goes away. I only use export default withRoot(withStyles(styles)(SomePage)) on files in ./pages. I only use export default withStyles(styles)(SomeComponent) on files in ./components.

When rendering a page, withRoot() should be called once at the very top of the tree. If it somehow gets called twice or withStyles() gets called before it, you will see the rendering issue and error in the log.

bendiy commented 6 years ago

Here's how I'm able to recreate this issue. Working with two examples here: The Next.js example MUI provides: https://github.com/mui-org/material-ui/blob/ac58d7b2c070cbd3d3df835063f56feed4944147/examples/nextjs/pages/index.js#L73 Adding in the Next.js Auth0 example here: https://github.com/luisrudge/next.js-auth0/blob/master/pages/index.js#L89

I get the rendering bug if my index.js line is:

export default withStyles(styles)(defaultPage(withRoot(Index)));

I do not get the rendering bug if my index.js line is:

export default withRoot(defaultPage(withStyles(styles)(Index)));

As I explained above, this bug happens when withRoot() and withStyles() Higher-Order Components are called in the wrong order. Make sure withRoot() is truly the first root of the page.

bendiy commented 6 years ago

This is the error I get in the browser console:

warning.js?97bab38:33 Warning: Prop `className` did not match. Server: "MuiTypography-root-106 MuiTypography-display3-108 MuiTypography-gutterBottom-123 MuiTypography-alignCenter-119" Client: "MuiTypography-root-215 MuiTypography-display3-217 MuiTypography-gutterBottom-232 MuiTypography-alignCenter-228"

It renders fine the first time after restarting the next.js server. If I then refresh the browser, It looses it's style and renders weird and throws the error.

oliviertassinari commented 6 years ago

It renders fine the first time after restarting the next.js server. If I then refresh the browser, It looses it's style and renders weird and throws the error.

@bendiy It's the sign your class name generator is shared between different request. Don't do it.

up209d commented 6 years ago

I got that warning as well

Warning: Prop className did not match. Server: "MuiButton-raisedPrimary-80" Client: "MuiButton-raisedPrimary-20"

It is definitely because of the className generator is shared among requests. So the generator function should be placed inside handler request function to avoid global variable

app.use(function(req, res) {
  const generateClassName = createGenerateClassName();
  // Pass it through props of a root component
});

...

<JssProvider registry={sheets} generateClassName={props.generateClassName}>

Otherwise if you put it somewhere it has to be a function

const generateClassName = () => {
  return createGenerateClassName();
};

...

<JssProvider registry={sheets} generateClassName={generateClassName()}>
dandrei commented 6 years ago

UPDATE Adding the code specified in the official MUI Next.js example did the trick: https://github.com/mui-org/material-ui/tree/v1-beta/examples/nextjs

Next.js: 5.0.0 Material-UI: 1.0.0-beta.34

I'm using withStyles the same way I'm using it in a CRA app, namely:

export default withStyles(styles)(Layout);

I apply it to a component under ./layouts/, which I use as a template for files under ./pages/

On the first load when Next.js is started, there are no errors. When refreshing, I start getting:

main.js:4542 Warning: Prop className did not match. Server: (...)

Any clue on how to address this?

buesing commented 6 years ago

I'm also experiencing this for the second time, seemingly at random. Same repos, same node and npm versions, but one is only rendering the 'jss' selectors on the server side, so the material-ui styles are not applied.

oliviertassinari commented 6 years ago

@buesing I have seen this "type" of issue many times now. It was always a configuration issue on user land. It would be best to solve this issue on StackOverflow so more people can benefit from it.

krazyjakee commented 6 years ago

@bendiy I then refresh the browser, It looses it's style and renders weird and throws the error I have this exact issue.

@oliviertassinari I started a project recently and was looking at after.js and next.js as being the source of this issue, but I think the closest I've come to an actual solution is your comment: It's the sign your class name generator is shared between different request. Could you please explain where in a project I should be declaring createGenerateClassName and how to make the styling stick on further requests?

My code:

static async getInitialProps({ assets, data, renderPage }) {
    const sheets = new SheetsRegistry();
    const generateClassName = createGenerateClassName();

    const page = await renderPage(After => props => <JssProvider registry={sheets} generateClassName={generateClassName}>
      <Layout><After {...props} /></Layout>
    </JssProvider>
    );
    return { assets, data, sheets, ...page };
  }