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

[examples] Create nextjs example using styled-components #27088

Merged
merged 20 commits into from
Jul 22, 2021

Conversation

hboylan
Copy link
Contributor

@hboylan hboylan commented Jul 3, 2021

Fixes #27087

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 3, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 6ae845b

@eps1lon eps1lon changed the title docs: create nextjs example using styled-components [examples] create nextjs example using styled-components Jul 5, 2021
@eps1lon eps1lon added the docs Improvements or additions to the documentation label Jul 5, 2021
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Anything left to do?

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not currently working. The PR is opened to illustrate the problem as stated in #27087 (comment) Will look into it later today.

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

Just updated _document.tsx to match the official next with-styled-components example, but still no luck. I'm not sure if MUI requires something to be done on the server in addition to the styled-components logic.

@mnajdova
Copy link
Member

mnajdova commented Jul 5, 2021

Just updated _document.tsx to match the official next with-styled-components example, but still no luck. I'm not sure if MUI requires something to be done on the server in addition to the styled-components logic.

There shouldn't be anything else required, the create-react-app-with-styled-components-typescript doesn't need anything else. I tried adding webpack5: false in next.config.js, but that didn't help either. On our docs, we use the babel.config.js for configuring the alias.

On an unrelated note, I would maybe update the README.md to:

index ac99c62162..d2230dae29 100644
--- a/examples/nextjs-with-styled-components-typescript/README.md
+++ b/examples/nextjs-with-styled-components-typescript/README.md
@@ -22,7 +22,7 @@ or:

 ## The idea behind the example

-The project uses [Next.js](https://github.com/zeit/next.js), which is a framework for server-rendered React apps. It includes `@material-ui/core` and its peer dependencies, including `emotion`, the default style engine in Material-UI v5. If you prefer, you can [use styled-components instead](https://next.material-ui.com/guides/interoperability/#styled-components).
+The project uses [Next.js](https://github.com/zeit/next.js), which is a framework for server-rendered React apps. It includes `@material-ui/core` and its peer dependencies, including `styled-components`, which is used instead of the default style engine in Material-UI v5.

 ## The link component

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

@mnajdova I made the recommended change to the README.

The CRA example you referenced is entirely client-side rendered, but NextJS relies on server-side rendering. This makes me think the ServerStyleSheets referenced in the v4 docs will be required.
https://material-ui.com/guides/server-rendering/#handling-the-request

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

This fixes the original error Error: Cannot find module '@emotion/styled'.

Now, the example renders when the app initially starts. It has proper styling, but warns:

Warning: Expected server HTML to contain a matching <div> in <div>.

Then, if the app reloads for any reason, the styles are missing with the slightly different warning:

Warning: Did not expect server HTML to contain a <style> in <div>.

🤔

@mnajdova
Copy link
Member

mnajdova commented Jul 5, 2021

This fixes the original error Error: Cannot find module '@emotion/styled'.

If I need to guess, it would be the changes in .babelrc that fixed the issue.

Now, the example renders when the app initially starts. It has proper styling, but warns:

Warning: Expected server HTML to contain a matching

in
.
Then, if the app reloads for any reason, the styles are missing with the slightly different warning:

Warning: Did not expect server HTML to contain a <style> in

.
🤔

How about we remove the dependency on @material-ui/styles, and any code associated with it? Ideally, we wouldn't like to bundle JSS by default in the examples.

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

According to the docs, it's necessary to import ServerStyleSheets from @material-ui/styles to support SSR.

The ServerStyleSheets component is no longer exported from @material-ui/core/styles. You should import it directly from @material-ui/styles.

Edit: Looks like the babel changes fixed it like you said and @material-ui/styles isn't necessary. 👍🏻

I was able to resolve the issue where styles go missing on reload by removing StyledEngineProvider, but the initial warning still persists.

Warning: Expected server HTML to contain a matching <div> in <div>.

@mnajdova
Copy link
Member

mnajdova commented Jul 5, 2021

According to the docs, it's necessary to import ServerStyleSheets from @material-ui/styles to support SSR.

The ServerStyleSheets component is no longer exported from @material-ui/core/styles. You should import it directly from @material-ui/styles.

https://next.material-ui.com/guides/migration-v4/#material-ui-core-styles

I see the confusion, we should update the docs to state that this is only required if JSS is used.

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

The warning appears related to babel-plugin-styled-components, but I can't seem to squash the issue.

By adding a unique identifier to every styled component, this plugin avoids checksum mismatches due to different class generation on the client and on the server. If you do not use this plugin and try to server-side render styled-components React will complain with an HTML attribute mismatch warning during rehydration.

@mnajdova
Copy link
Member

mnajdova commented Jul 5, 2021

@hboylan I've just tried again the example, and I am still seeing

Server Error
Error: Cannot find module '@emotion/styled'
Require stack:
- C:\Users\mnajd\Desktop\examples tests\2021-07-05\nextjs-with-styled-components-typescript\node_modules\@material-ui\styled-engine\node\index.js

It may be related to the latest changes in .babelrc

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

Hmm. I get that error again after removing the root node_modules. I had initially installed the root dependencies to run yarn lint && yarn prettier without realizing the example project would pick them up. 😞

@hboylan
Copy link
Contributor Author

hboylan commented Jul 5, 2021

As far as I can tell, @material-ui/styled-engine is not being replaced by @material-ui/styled-engine-sc because the final stack item references it.

Full stack
Error: Cannot find module '@emotion/styled'
Require stack:
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/@material-ui/styled-engine/node/index.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/@material-ui/system/index.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/@material-ui/core/node/styles/adaptV4Theme.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/@material-ui/core/node/styles/index.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/.next/server/pages/_document.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/next-server/server/require.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/next-server/server/load-components.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/next-server/server/api-utils.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/next-server/server/next-server.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/server/next.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/server/lib/start-server.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/cli/next-dev.js
- /Users/hugh/Development/node/material-ui/examples/nextjs-with-styled-components-typescript/node_modules/next/dist/bin/next

@mnajdova
Copy link
Member

@hboylan are there other things to be addressed? I've reverted the changes outside of the example.

I could not spot any issues in the production build (I've tested with both JS enabled and disabled).

In addition, is the .eslintrc necessary in the new example? Can we omit it? Looks like yarn lint is currently failing :\

@hboylan
Copy link
Contributor Author

hboylan commented Jul 19, 2021

@mnajdova New Next.js projects are initialized with .eslintrc, but I initially left it out of the example because it doesn't play well inside the MUI repo for some reason. You had added it, but I don't think it's necessary for an example project.

Otherwise, I just want to be sure the SSR warning is resolved. Right now I'm seeing it again. Is anyone else able to reproduce?

  1. pull latest changes
  2. rm -rf .next
  3. yarn dev
  4. Warning: Prop `className` did not match.

@mnajdova
Copy link
Member

@mnajdova New Next.js projects are initialized with .eslintrc, but I initially left it out of the example because it doesn't play well inside the MUI repo for some reason. You had added it, but I don't think it's necessary for an example project.

Ah right, sorry my bad, let's remove it then, it was there just because I started from a new project 👍 Good catch!

Otherwise, I just want to be sure the SSR warning is resolved. Right now I'm seeing it again. Is anyone else able to reproduce

Is it happening in dev mode only? I tested prod build there weren't any issues. Is it related to Material-UI? Can we reproduce it with styled-components only project?

@hboylan
Copy link
Contributor Author

hboylan commented Jul 19, 2021

Removed the eslint config.

yarn build && yarn start works just fine, but yarn dev logs the warning on page refresh. I believe it's an SSR-only issue.

@mnajdova
Copy link
Member

yarn build && yarn start works just fine, but yarn dev logs the warning on page refresh. I believe it's an SSR-only issue.

Did you test if disabled JavaScript works in dev and prod? I would say if it works we can merge, as it seems like it is only a warning (I know it's not ideal, but we can solve it later if there is an actual bug that something is not working).

@mnajdova
Copy link
Member

@hboylan I've tested again - with disabled JavaScript, both dev and prod are working fine, which would mean SSR is working. I think we can ignore the warning in dev mode for now.

@hboylan
Copy link
Contributor Author

hboylan commented Jul 19, 2021

That works for me. It's just odd to me that it's a sporadic warning.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hboylan a lot for initiating this effort :)

@mnajdova
Copy link
Member

That works for me. It's just odd to me that it's a sporadic warning.

We can always improve in the future. We are now at least unlocking developers who cannot get started with configuring nextjs with styled-components and Material-UI :)

@eps1lon
Copy link
Member

eps1lon commented Jul 20, 2021

I think we can ignore the warning in dev mode for now.

That is a blocking issue since it means concurrent rendering is also likely broken.

@mnajdova
Copy link
Member

That is a blocking issue since it means concurrent rendering is also likely broken.

@eps1lon ok, I agree, I spent one more hour on this and got back to vercel/next.js#7322. I remember we had the same issue on our docs, it's still there if we run it with styled-components. Also, comments from the author like this - vercel/next.js#7322 (comment) doesn't look like the issue will be resolved soon.

We can keep the PR open, or close it, or merge it with link to the issue in the readme that will explain why the warning is there. I hope that if we merge, it's more likely that someone from the community will fix it. In contrast, it we don't merge the example, we are likely going to receive an issue about this again.

Let me know what you think. Again, the warning is not showing in production.

@eps1lon
Copy link
Member

eps1lon commented Jul 22, 2021

@eps1lon ok, I agree, I spent one more hour on this and got back to vercel/next.js#7322.

I would encourage opening a new issue with the latest dependencies in styled-components. The issue linked is based off of older versions. The comments after closing are unhelpful and I would ignore them as well if I were a maintainer of styled-components.

Let me know what you think. Again, the warning is not showing in production.

It's a dev-only warning. Obviously it doesn't show in production.

We should have an issue open about StrictMode incompatibility of styled-components.

@mnajdova
Copy link
Member

I found this babel config that should fix the issue of styled-components and nextjs.

{
  "env": {
    "development": {
      "presets": ["next/babel"],
      "plugins": [
        [
          "babel-plugin-styled-components",
          { "ssr": true, "displayName": true, "preprocess": false }
        ]
      ]
    },
    "production": {
      "plugins": [
        [
          "babel-plugin-styled-components",
          { "ssr": true, "displayName": true, "preprocess": false }
        ]
      ],
      "presets": ["next/babel"]
    },
    "test": {
      "presets": ["next/babel"]
    }
  },
  "plugins": [
    [
      "babel-plugin-styled-components",
      { "ssr": true, "displayName": true, "preprocess": false }
    ]
  ]
}

However, I think I understand why it is not working in our example - styled-components/babel-plugin-styled-components#230 The import to the styled utility is not coming from styled-components so it is ignored by the plugin. We've got similar issue for emotion too - #26366

I will look into this next week. See if we can re-use the existing plugins or need new ones. Would be great if these plugins supported whitelist imports that the plugin could consider.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into this next week. See if we can re-use the existing plugins or need new ones. Would be great if these plugins supported whitelist imports that the plugin could consider.

Thanks for looking into it. Merging problematic code isn't a problem. But we do need to understand the problem and have a plan. Otherwise we're just piling on technical debt.

@mnajdova
Copy link
Member

Thanks for looking into it. Merging problematic code isn't a problem. But we do need to understand the problem and have a plan. Otherwise we're just piling on technical debt.

Agree, lesson learned for me :) At least we know what is the issue right now and can plan fix for it

@mnajdova mnajdova merged commit 6cf14fd into mui:next Jul 22, 2021
@oliviertassinari oliviertassinari changed the title [examples] create nextjs example using styled-components [examples] Create nextjs example using styled-components Jul 22, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 22, 2021

Should we link this new example in https://next.material-ui.com/getting-started/example-projects/#official-examples?


I'm curious. Did we consider this solution?

diff --git a/next.config.js b/next.config.js
deleted file mode 100644
index bd89a75..0000000
--- a/next.config.js
+++ /dev/null
@@ -1,12 +0,0 @@
-const withTM = require('next-transpile-modules')(['@material-ui/core', '@material-ui/system']); // pass the modules you would like to see transpiled
-
-module.exports = withTM({
-  reactStrictMode: true,
-  webpack: (config) => {
-    config.resolve.alias = {
-      ...config.resolve.alias,
-      '@material-ui/styled-engine': '@material-ui/styled-engine-sc',
-    };
-    return config;
-  },
-});
diff --git a/package.json b/package.json
index 38148b0..7943d0a 100644
--- a/package.json
+++ b/package.json
@@ -10,9 +10,8 @@
   },
   "dependencies": {
     "@material-ui/core": "next",
-    "@material-ui/styled-engine-sc": "next",
+    "@material-ui/styled-engine": "npm:@material-ui/styled-engine-sc@next",
     "next": "latest",
-    "next-transpile-modules": "latest",
     "react": "latest",
     "react-dom": "latest",
     "react-is": "latest",
diff --git a/tsconfig.json b/tsconfig.json
index 442fc43..750d409 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -13,9 +13,6 @@
     "resolveJsonModule": true,
     "isolatedModules": true,
     "jsx": "preserve",
-    "paths": {
-      "@material-ui/styled-engine": ["./node_modules/@material-ui/styled-engine-sc"] // this mapping is relative to "baseUrl"
-    }
   },
   "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx"],
   "exclude": ["node_modules"]

It seems to work, package alias are supported by npm since 6.9.0. I also can't trigger the warnings in dev mode.

The same question would go for https://github.com/mui-org/material-ui/tree/next/examples/create-react-app-with-styled-components and https://next.material-ui.com/guides/styled-engine/#how-to-switch-to-styled-components.

@mnajdova
Copy link
Member

@oliviertassinari I am getting this error when trying to run yarn dev when the propose changes are applied:

yarn run v1.22.5
$ next dev
ready - started server on 0.0.0.0:3000, url: http://localhost:3000
info  - Using webpack 5. Reason: Enabled by default https://nextjs.org/docs/messages/webpack5
error - ./node_modules/@material-ui/system/node_modules/@material-ui/styled-engine/GlobalStyles/GlobalStyles.js:3:0
Module not found: Can't resolve '@emotion/react'
null


Posting some more investigation. Looks like babel-plugin-styled-components has an option for setting more top level imports, like:

 [
      "babel-plugin-styled-components",
      { 
        "ssr": true, "displayName": true,
        "preprocess": false,
        "topLevelImportPaths": ["@material-ui/styled-engine", "@material-ui/styled-engine-sc", "@material-ui/core/styles"] }
    ]

But this didn't solve the issue, probably because the code itself lives in node_modules/@material-ui/core :\

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 25, 2021

@mnajdova Oh yeah, I tested it with npm but not yarn. For npm + yarn support, we need an extra:

     "react-dom": "latest",
     "react-is": "latest",
     "styled-components": "latest"
   },
+  "resolutions": {
+    "@material-ui/styled-engine": "npm:@material-ui/styled-engine-sc@next"
+  },
   "devDependencies": {

to make it work.

@agboolaidris
Copy link

I am getting this error in MUI 5

warning prop classname did not match. server material ui

@amaralc

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[examples] Add example to alias styled-components with Next.js
8 participants