Material-ui: Warning "Material-UI: this elevation `1` is not implemented" with disableGeneration to true

[Romcol]

• [X] The issue is present in the latest release.
• [X] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Expected Behavior 🤔

No warning should occur.

Steps to Reproduce 🕹

This issue happens in SSR. On the server.
We use ServerStyleSheets with disableGeneration to true (for performance gains).

const sheetsPrepass = new ServerStyleSheets({ disableGeneration: true });
and wrap the react element such as this sheetsPrepass.collect(reactElement).

The issue occurs only when disableGeneration is set to true.

I think this is due to this. classes is empty with disableGeneration on the server, while it's not in other cases. I'm not sure how it should be fixed though.

https://github.com/mui-org/material-ui/blob/51b2b927cf31b59f16dbe7d2a51ef1ee111845cc/packages/material-ui/src/Paper/Paper.js#L44-L49

I don't think a codesandbox si necessary here as most of the information is there.

Thank you :rocket: !

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.9.5 |
| React | v16.12.0 |
| Browser | NodeJS on the server |

[eps1lon]

Yeah we should not warn in that case. It's a bit tricky to recognize the context though.

It's not a high priority issue though since the use case isn't one that is recommended by react anyway. You shouldn't introspect the rendered tree anyway.

[oliviertassinari]

@eps1lon What do you think of going back to the theme approach in https://github.com/mui-org/material-ui/pull/18385/files#r346834275? It would solve this issue and help us when moving to styled components.

[Romcol]

Yeah we should not warn in that case. It's a bit tricky to recognize the context though.

It's not a high priority issue though since the use case isn't one that is recommended by react anyway. You shouldn't introspect the rendered tree anyway.

Sure it's low priority, it doesn't show up when compiled to production. What do you mean I shouldn't introspect the rendered tree?
I have to use react-ssr-prepass to get the list of components rendered so as to make the API calls synchronous (getInitialProps() like in next-js). In this respect, I have to introspect the rendered tree while disabling the generation of style. I think it also applies to react-apollo.
I'd be glad to hear from you guys if you think I shouldn't do that, advice from more react-experienced developers is always beneficial :wink:

[oliviertassinari]

@Romcol If you want to give it a try, I think that we could get away with this patch:

diff --git a/packages/material-ui/src/Paper/Paper.js b/packages/material-ui/src/Paper/Paper.js
index 201872f00..142cc528a 100644
--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -2,6 +2,7 @@ import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
+import useTheme from '../styles/useTheme';

 export const styles = theme => {
   const elevations = {};
@@ -42,7 +43,9 @@ const Paper = React.forwardRef(function Paper(props, ref) {
   } = props;

   if (process.env.NODE_ENV !== 'production') {
-    if (classes[`elevation${elevation}`] === undefined) {
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    const theme = useTheme();
+    if (!theme.shadows[elevation]) {
       console.error(`Material-UI: this elevation \`${elevation}\` is not implemented.`);
     }
   }

pull request welcome :).

Now, assuming we move to styled-components, we will no longer be able to leverage the classes object to run this warning. Also, assuming we start to support dynamic values from the theme, I think that we start to need a broader solution to the problem. For instance, let's say a developer forgot to add danger color in its palette, we will need to run a warning when doing <Button color="danger" />. But it's a problem to solve for another day :).

[nesso-pfl]

I'll work on this issue if it's okay👀

[oliviertassinari]

@nesso-pfl Thanks for the interest in this issue. Considering the direction we are taking with the styles, I think that it makes sense to implement this warning, accessing the theme directly, not relying on the generates classes (as its done now).

[nesso-pfl]

Well, it's done, isn't it?
So I'll look for another good-first-issue. Thank you 😃
(Sorry for my poor understanding English.)

[oliviertassinari]

As far as I know, the problem still stand. We rely on the classes, while we should depend on the theme.

[nesso-pfl]

I think what should I do is to use the theme instead of the classes as shown below, right?
https://github.com/mui-org/material-ui/compare/next...nesso-pfl:use-theme-instead-of-classes?expand=1

[oliviertassinari]

@nesso-pfl I don't think that we can use the prop-type check anymore. Something in this order (maybe need to improve the warning message)

diff --git a/packages/material-ui/src/Paper/Paper.js b/packages/material-ui/src/Paper/Paper.js
index b1431b60d4..6cf24d052b 100644
--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -1,9 +1,9 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
-import { chainPropTypes } from '@material-ui/utils';
 import { useThemeVariants } from '@material-ui/styles';
 import withStyles from '../styles/withStyles';
+import useTheme from '../styles/useTheme';

 export const styles = (theme) => {
   const elevations = {};
@@ -56,6 +56,14 @@ const Paper = React.forwardRef(function Paper(props, ref) {
     'MuiPaper',
   );

+  if (process.env.NODE_ENV !== 'production') {
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    const theme = useTheme();
+    if (theme.shadows[elevation] === undefined) {
+      console.warn(`Material-UI: The elevation provided <Paper elevation={${elevation}}> is not available in the theme.`);
+    }
+  }
+
   return (
     <Component
       className={clsx(
@@ -101,17 +109,7 @@ Paper.propTypes = {
    * It accepts values between 0 and 24 inclusive.
    * @default 1
    */
-  elevation: chainPropTypes(PropTypes.number, (props) => {
-    const { classes, elevation } = props;
-    // in case `withStyles` fails to inject we don't need this warning
-    if (classes === undefined) {
-      return null;
-    }
-    if (elevation != null && classes[`elevation${elevation}`] === undefined) {
-      return new Error(`Material-UI: This elevation \`${elevation}\` is not implemented.`);
-    }
-    return null;
-  }),
+  elevation: PropTypes.number,
   /**
    * If `true`, rounded corners are disabled.
    * @default false