-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[examples] Create nextjs example using styled-components #27088
Conversation
There was a problem hiding this 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?
There was a problem hiding this 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.
Just updated |
There shouldn't be anything else required, the create-react-app-with-styled-components-typescript doesn't need anything else. I tried adding 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
|
@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 |
This fixes the original error Now, the example renders when the app initially starts. It has proper styling, but warns:
Then, if the app reloads for any reason, the styles are missing with the slightly different warning:
🤔 |
If I need to guess, it would be the changes in .babelrc that fixed the issue.
How about we remove the dependency on |
Edit: Looks like the babel changes fixed it like you said and I was able to resolve the issue where styles go missing on reload by removing
|
I see the confusion, we should update the docs to state that this is only required if JSS is used. |
The warning appears related to
|
@hboylan I've just tried again the example, and I am still seeing
It may be related to the latest changes in |
Hmm. I get that error again after removing the root |
As far as I can tell,
|
@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 |
@mnajdova New Next.js projects are initialized with 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?
|
Ah right, sorry my bad, let's remove it then, it was there just because I started from a new project 👍 Good catch!
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 |
Removed the eslint config.
|
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). |
@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. |
That works for me. It's just odd to me that it's a sporadic warning. |
There was a problem hiding this 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 :)
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 :) |
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 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. |
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.
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. |
I found this babel config that should fix the issue of
However, I think I understand why it is not working in our example - styled-components/babel-plugin-styled-components#230 The import to the 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. |
There was a problem hiding this 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.
Agree, lesson learned for me :) At least we know what is the issue right now and can plan fix for it |
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. |
@oliviertassinari I am getting this error when trying to run
Posting some more investigation. Looks like
But this didn't solve the issue, probably because the code itself lives in |
@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. |
I am getting this error in MUI 5 warning prop classname did not match. server material ui |
Fixes #27087