Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

makeStyles with a function css rule entry doesn't work on react 18 using react-dom/client createRoot #32881

Closed
2 tasks done
rart opened this issue May 23, 2022 · 8 comments
Closed
2 tasks done
Assignees
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it

Comments

@rart
Copy link
Contributor

rart commented May 23, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

On react 18, when rendering using ReactDomClient.createRoot().render(...), using function css rule definitions doesn't work correctly — styles are missing from the runtime, despite the classes being applied to the elements.

Still on react 18, rendering via ReactDom.render does work.

const useStyles = makeStyles(() =>
  createStyles({
    // Function style rule: Doesn't work
    fnStyleClass: () => {
      return {
        color: "red",
        background: "black"
      };
    },
    // Plain object: Works
    objStyleClass: {
      color: "red",
      background: "black"
    }
  })
);

This is particularly problematic because this is the new default of create react app.

Expected behavior 🤔

Function style rules work and apply just like plain objects independently of the react render method used.

Steps to reproduce 🕹

Steps:

  1. Create a new react app with the lates CRA (e.g. yarn create react-app make-styles-test)
  2. Use makeStyles and define a rule using a function callback instead of a plain object
  3. See styles aren't applied during runtime

Live example @ https://codesandbox.io/s/mui-makestyles-functions-issue-jhjfq5

The example renders two Typography components; each with a class produced by a makeStyles({})(). One of the Typography elements takes a class for a style rule body returned by a function and the other from a plain object. The style rules are identical, the only difference is the plain object vs function definitions. When rendered via createRoot.render, you can see one of the Typography elements doesn't have the styling that the class defines (black background, red font color).

Notice at the bottom of the index.tsx file the two ways of rendering (ReactDom.render vs ReactDomClient.createRoot.render). One works and the other one doesn't. You may comment/uncomment the two ways to see the different results.

Context 🔦

We have an npm package with components based on mui. Despite our package being on mui@^5, a number of our components haven't fully migrated out of the @mui/styles into the new style system. CRA latest uses createRoot().render() by default and, when creating a CRA app and using our package, a lot of our styles are simply missing.

The issue is not specific to our package (see clean codesandbox example).

Your environment 🌎

Chrome & Safari. Probably all.

  System:
    OS: macOS 12.4
  Binaries:
    Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
    Yarn: 1.22.17 - ~/.nvm/versions/node/v16.13.0/bin/yarn
    npm: 8.1.4 - ~/.nvm/versions/node/v16.13.0/bin/npm
  Browsers:
    Chrome: 101.0.4951.64
    Edge: Not Found
    Firefox: 99.0
    Safari: 15.5
  npmPackages:
    @emotion/react: ^11.5.0 => 11.9.0 
    @emotion/styled: ^11.3.0 => 11.8.1 
    @mui/base:  5.0.0-alpha.82 
    @mui/icons-material: ^5.3.0 => 5.3.0 
    @mui/lab: ^5.0.0-alpha.65 => 5.0.0-alpha.83 
    @mui/material: ^5.3.0 => 5.8.1 
    @mui/private-theming:  5.8.0 
    @mui/styled-engine:  5.8.0 
    @mui/styles: ^5.3.0 => 5.8.0 
    @mui/system:  5.8.1 
    @mui/types:  7.1.3 
    @mui/utils:  5.8.0 
    @mui/x-data-grid:  5.11.1 
    @mui/x-date-pickers:  5.0.0-alpha.1 
    @types/react: ^18.0.0 => 18.0.9 
    react: ^18.1.0 => 18.1.0 
    react-dom: ^18.1.0 => 18.1.0 
    typescript: ^4.4.2 => 4.6.4 
@rart rart added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 23, 2022
@garronej
Copy link
Contributor

Hi @rart,
The implementation of makeStyles you are using is deprecated.
I suggest you upgrade to tss-react.
On your sandbox it would be: https://codesandbox.io/s/mui-issue-with-makestyles-functions-forked-oftpd9?file=/src/Demo.tsx

Latest MUI version includes a codemod for automatic migration from JSS to TSS. https://mui.com/material-ui/guides/migration-v4/#2-use-tss-react

Best regards

@rart
Copy link
Contributor Author

rart commented May 23, 2022

Thanks for the update, @garronej

Back when we did the migration to mui v5 (very early on), tss-react wasn't an option or part of the upgrade guide. I guess it's also just slightly misleading to keep seeing version updates on @mui/styles.

I did just try the codemod and it doesn't work over a previously updated codebase. Because we had already upgraded to MUI v5, our makeStyles import statements are from @mui/styles/makeStyles and not from @material-ui/styles/makeStyles like the codemod expects. It also doesn't seem to remove the createStyles call and I get some additional compilation errors that I need to inspect further to understand.

Is there a post v5 upgrade codemod that would work on already updated codebases? Or that helps with the other issues too?

Also, being @mui/styles deprecated I assume it means this ticket won't get fixed?

@garronej
Copy link
Contributor

It also doesn't seem to remove the createStyles call and I get some additional compilation errors that I need to inspect further to understand.

I didn't write the codemod but there is at least one test case that does remove createStyle.
https://github.com/mui/material-ui/pull/31802/files#diff-b13e5553c5f57f8e729ca91e76b03e68e84e04bb15d2723ea928a735c0b826bb

@mui/styles/makeStyles and not from @material-ui/styles/makeStyles like the codemod expects

If it's only that you could try a search/replace and run the codemod again.

Anyway, the migration is quite easy to perform by hand on a typescript codebase, assuming it isn't huge.

Also, being @mui/styles deprecated I assume it means this ticket won't get fixed?

I woudn't count to much on it. Ref.

Good luck!

@rart
Copy link
Contributor Author

rart commented May 25, 2022

Correction: the codemod does remove the createStyles — I just made a mistake when I tested it.

We did the migration to tss and it was a lot of manual tweaking even with the codemod but the problem is gone so looks like the issue is indeed related to the incompatibility of @mui/styles with concurrent mode.

I'll leave the ticket open for you guys to decide if you want to address or close.

@garronej
Copy link
Contributor

thanks for the feedback!

@ryancogswell
Copy link
Contributor

@rart FYI - @joshkel did enhance the codemod to support replacing @mui/styles/makeStyles (#32962).

I wrote the initial version of the jss-to-tss-react codemod. If there are some patterns that were common in your code that it didn't cover, please share some code examples. I'll be able to tell fairly quickly whether or not it's a pattern that I could easily enhance the codemod to cover.

@rart
Copy link
Contributor Author

rart commented Jun 21, 2022

@ryancogswell,

@rart FYI - @joshkel did enhance the codemod to support replacing @mui/styles/makeStyles (#32962).

We already migrated, but that's a great addition 👏

I wrote the initial version of the jss-to-tss-react codemod. If there are some patterns that were common in your code that it didn't cover, please share some code examples. I'll be able to tell fairly quickly whether or not it's a pattern that I could easily enhance the codemod to cover.

Because the migration was done some weeks ago, I've now forgotten some of the struggles or some details. But here's what I remember:

  1. We were on mui/styles instead of @material-ui/styles (already covered, as you mentioned)
import createStyles from '@mui/styles/createStyles';
import makeStyles from '@mui/styles/makeStyles';
  1. We had this pattern in many places:
const useStyles = makeStyles((theme) =>
   createStyles<ViewToolbarClassKey, ViewToolbarStyles>({
     appBar: (styles) => ({
       borderBottom: `1px solid ${theme.palette.divider}`
       ...styles.appBar
     }),

Notice how the rule is a function that receives an arg that's not destructured. The codemod only worked if the argument was destructured. We had to covert the above to the below before running the codemod:

const useStyles = makeStyles((theme) =>
   createStyles<ViewToolbarClassKey, ViewToolbarStyles>({
     appBar: ({ appBar }) => ({
       borderBottom: `1px solid ${theme.palette.divider}`
       ...appBar
     }),

Other things I remember vaguely is that there were some additional strange things post codemod. For example,

  • The useStyles in some cases was modified to be invoked with two arguments (i.e. the props as the second argument instead of as the first).
  • The types used in createStyles<ViewToolbarClassKey, ViewToolbarStyles>({ were not fully migrated to the modified code.

@mnajdova
Copy link
Member

The @mui/styles package is not compatible with React 18, as shown in the documentation - https://mui.com/system/styles/basics/ You should migrate to either @mui/system and use styled(), sx etc, or migrate to tss-react which has an API which is closer to @mui/styles. I hope this expalantion helps other who may come to this issue.

@mnajdova mnajdova added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it
Projects
None yet
Development

No branches or pull requests

5 participants