search

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
92.45k stars 31.85k forks source link

createGenerateClassName reliance on NODE_ENV with SSR creates issues #14228

Closed fuzing closed 5 years ago

fuzing commented 5 years ago

material-ui/core 3.9.0 (and prior versions)

The current version of createGenerateClassName (@material-ui/core/src/styles/createGenerateClassName.js) generates different classNames based on the current NODE_ENV setting. This seems to be causing numerous folks to report className mismatches when doing SSR, and seems like it may be the cause of multiple bug reports.

A problem arises if the server is running (say) "production" and the client is bundled with a "development" build (or vice-versa). Hydrating the client will result in className mismatches and missing styles. It's probably preferable that the className generator is decoupled from NODE_ENV.

Thanks!

oliviertassinari commented 5 years ago

@fuzing Yes, the client and the server should use the same NODE_ENV. I don't think that it's a problem, quite the other way around. I think that people should be welcome to Material-UI for highlighting this important discrepancy.

fuzing commented 5 years ago

I think it's fine if documented, but there are legitimate use cases such as during debugging, frontend vs. Backend node_env settings are often set differently. To have the ui become dysfunctional compounds this with an unecessary distraction. It looks like there are half a dozen issues that have been opened here, at cssinjs/jss and even on the create-react-app repo that look like they went unresolved, but are clearly rooted in this very issue. It's bound to be a recurring theme. The point is that material-ui doesn't highlight the problem ....... React complains about the DOM mismatch (and only when browser/client is using the dev build of react-dom, otherwise it just silently renders without matching CSS). If material ui could detect and report in a meaningful way then I'd agree...... that would be very useful. Thanks for your hard work.

oliviertassinari commented 5 years ago

but are clearly rooted in this very issue. It's bound to be a recurring theme

@fuzing Yes, the JSS index counter-strategy has been a recurring issue for people doing server-side rendering. cc @kof. Many people struggle to get the logic right, it's hard. The ENV issue is a small fraction of what could go wrong, I would say 10%. It's documented in https://material-ui.com/guides/server-rendering/#react-class-name-hydration-mismatch. There are many other potential issues. Here is a new one with a wrong react-jss version: #14219.

Now, because of all these feedbacks, we have decided to use a different strategy than in react-jss. In @material-ui/styles, we are using a hash-based strategy, it's stable, it doesn't depend on the call order.

I think it's fine if documented

https://material-ui.com/css-in-js/advanced/#class-names

If material ui could detect and report in a meaningful way

Yeah, this would be awesome. But React doesn't allow us to hook into it's hydratation mismatches 🤔. Hopefully, this issue can be found by others.

kof commented 5 years ago

I have actually a solid plan on how to make a better error reporting.

Also there might be a way to use hashes with dynamic rules, but it needs to be worked on.

On Fri, Jan 18, 2019, 10:03 Olivier Tassinari <notifications@github.com wrote:

but are clearly rooted in this very issue. It's bound to be a recurring theme

Yes, the JSS index counter-strategy has been a recurring issue for people doing server-side rendering. cc @kof https://github.com/kof. Many people struggle to get logic right, it's hard. The ENV issue is a small fraction of what could go wrong, I would say 10%. It's documented in https://material-ui.com/guides/server-rendering/#react-class-name-hydration-mismatch. There are many other potential issues. Here is a new one with a wrong react-jss version: #14219 https://github.com/mui-org/material-ui/issues/14219.

Now, because of all these feedbacks, we have decided to use a different strategy than in react-jss. In @material-ui/styles, we are using a hash-based strategy, it's stable, it doesn't depend on the call order.

I think it's fine if documented

https://material-ui.com/css-in-js/advanced/#class-names

If material ui could detect and report in a meaningful way

Yeah, this would be awesome. But React doesn't allow us to hook into it's hydratation mismatches 🤔. Hopefully, this issue can be found by others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mui-org/material-ui/issues/14228#issuecomment-455474591, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWLoLZTVY_iRkFp2aYHlcbsx2pEJNks5vEY3wgaJpZM4aG2Kg .

fuzing commented 5 years ago

Sounds like a good plan - thanks for the feedback!

On Fri, Jan 18, 2019 at 2:24 AM Oleg Isonen notifications@github.com wrote:

I have actually a solid plan on how to make a better error reporting.

Also there might be a way to use hashes with dynamic rules, but it needs to be worked on.

On Fri, Jan 18, 2019, 10:03 Olivier Tassinari <notifications@github.com wrote:

but are clearly rooted in this very issue. It's bound to be a recurring theme

Yes, the JSS index counter-strategy has been a recurring issue for people doing server-side rendering. cc @kof https://github.com/kof. Many people struggle to get logic right, it's hard. The ENV issue is a small fraction of what could go wrong, I would say 10%. It's documented in

https://material-ui.com/guides/server-rendering/#react-class-name-hydration-mismatch . There are many other potential issues. Here is a new one with a wrong react-jss version: #14219 https://github.com/mui-org/material-ui/issues/14219.

Now, because of all these feedbacks, we have decided to use a different strategy than in react-jss. In @material-ui/styles, we are using a hash-based strategy, it's stable, it doesn't depend on the call order.

I think it's fine if documented

https://material-ui.com/css-in-js/advanced/#class-names

If material ui could detect and report in a meaningful way

Yeah, this would be awesome. But React doesn't allow us to hook into it's hydratation mismatches 🤔. Hopefully, this issue can be found by others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/mui-org/material-ui/issues/14228#issuecomment-455474591 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AADOWLoLZTVY_iRkFp2aYHlcbsx2pEJNks5vEY3wgaJpZM4aG2Kg

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mui-org/material-ui/issues/14228#issuecomment-455480559, or mute the thread https://github.com/notifications/unsubscribe-auth/ALHBFUNfCtikDmsOKlT7l3bOsFmDng52ks5vEZLCgaJpZM4aG2Kg .