-
-
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] Next.js v13 app router with Material UI #37315
Conversation
smo043
commented
May 18, 2023
- I have followed (at least) the PR section of the contributing guide.
Netlify deploy previewhttps://deploy-preview-37315--material-ui.netlify.app/ Bundle size report |
This change will make it easier for people who are trying to use Next App Router with Material UI v5. |
'use client'; | ||
|
||
import * as React from 'react'; | ||
import { Container, Box, Typography } from '@mui/material'; |
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 think that we can go with deep imports, for faster dev load time:
import { Container, Box, Typography } from '@mui/material'; | |
import Container from '@mui/material/Container'; | |
import Box from '@mui/material/Box'; | |
import Typography from '@mui/material/Typography'; |
x the rest of the demos
<!-- #default-branch-switch --> | ||
|
||
```sh | ||
curl https://codeload.github.com/mui/material-ui/tar.gz/master | tar -xz --strip=2 material-ui-master/examples/material-next-app-router-ts |
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 think that we would be better off with this demo as the default, pages
is legacy at this point.
curl https://codeload.github.com/mui/material-ui/tar.gz/master | tar -xz --strip=2 material-ui-master/examples/material-next-app-router-ts | |
curl https://codeload.github.com/mui/material-ui/tar.gz/master | tar -xz --strip=2 material-ui-master/examples/material-next-ts |
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.
Do we have to make any changes to the page directory url since we have defaulted to the app one?
modularizeImports: { | ||
'@mui/icons-material': { | ||
transform: '@mui/icons-material/{{member}}', | ||
}, | ||
}, |
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.
Yes, great. I think that we really need to spend time on #35457.
experimental: { | ||
typedRoutes: true, | ||
}, |
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.
To keep it more future-proof?
experimental: { | |
typedRoutes: true, | |
}, |
import * as React from 'react'; | ||
import { ThemeProvider } from '@mui/material/styles'; | ||
import CssBaseline from '@mui/material/CssBaseline'; | ||
import { NextAppDirEmotionCacheProvider } from 'tss-react/next/appDir'; |
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.
Why use this? https://github.com/garronej/tss-react/blob/b06eccf9017032536161053eefa12b559516b592/src/next/appDir.tsx#L12. I think that it would be better if we own the logic.
I assume it's similar to https://nextjs.org/docs/app/building-your-application/styling/css-in-js#styled-components. We could also look at how the current _document integration is done.
More benchmark:
@oliviertassinari - thank you, I will work on your review comments. |
export default function RootLayout({ children }: { children: React.ReactNode }) { | ||
return ( | ||
<html lang="en"> | ||
<body> |
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.
Does MUI work with next/font
?
import {Inter} from 'next/font/google';
const inter = Inter({subsets: ['latin']});
// ...
<body className={inter.className}>
...
</body>
If it does, would be really helpful to include it in the example.
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.
Yes it works with the Next fonts, I can include it in the example.
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.
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'm not part of the MUI team, but the current version of this PR looks great! Thanks for adding this example!
When merged, this will resolve the issue #34898
Setup the environment for testing and to leverage a component library for expedience of building the site. It should, also, be noted that MUI can only be used when a component is rendered client-side, instead of, the prefered server-side. This is issue accures as of Next.js 13, and is explained in further detail: mui/material-ui#34898, mui/material-ui#37315
@oliviertassinari @mnajdova - can we merge this pr? |
<React.Fragment> | ||
<CssBaseline /> | ||
<NextAppDirEmotionCacheProvider options={{ key: 'mui' }}> | ||
<ThemeProvider theme={theme}>{children}</ThemeProvider> |
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 believe <CssBaseline />
should be nested under ThemeProvider
, so that dark mode works correctly.
Many thanks for this PR btw!
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 actually have encountered an issue:
If I change the theme's palette mode at runtime. The Paper
and Button
components do reflect the change, but others like Typography
or CssBaseline
do not.
If I comment out NextAppDirEmotionCacheProvider
changing the mode at runtime works.
Edit: The problem went away after migrating to CssThemeVars.
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.
@kernelwhisperer Could you please share the sandbox link?
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.
Agree, the CssBaseline
should be under the ThemeProvider
.
I may be missing something, but this approach uses |
I have the same question. Could you please help me answer it? thanks |
@joeydqyuan @taschetto 'use client' does not mean only client-side rendering. It means that the JavaScript in the file is going to be bundled and sent to the client, while also rendering server side. So, when a file has 'use client' at the top of it, it "tells" next.js to also send the JavaScript to the client instead of only sending HTML and CSS. The bottom line is - 'use client' does not mean client side rendering. The content is still going to be rendered on the server (just like with |
This comment was marked as off-topic.
This comment was marked as off-topic.
We can take this separately. @smo043 thanks a lot for the contribution. Let me test it out a bit today and merge if everything works as expected :) |
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 looks great! I left a few comments. I would recommend adding one more page so that we show how the navigation would work. We have this as an example in all next.js example projects.
children: React.ReactNode; | ||
}; | ||
|
||
export function NextAppDirEmotionCacheProvider(props: NextAppDirEmotionCacheProviderProps) { |
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.
export function NextAppDirEmotionCacheProvider(props: NextAppDirEmotionCacheProviderProps) { | |
// This implementation is taken from https://github.com/garronej/tss-react/blob/main/src/next/appDir.tsx | |
export function NextAppDirEmotionCacheProvider(props: NextAppDirEmotionCacheProviderProps) { |
Ideally, this should come from emotion. @Andarist would the emotion time accept this as a contribution?
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 think so. I have to give this some more thought but:
- this is somewhat working and this solution is shared in our issues so everyone can copy-paste it anyway
- it likely has some subtle/edge-case issues but people usually don't run into them at all so I think it's roughly OK to be used right now
- it's a temporary solution - so it would have to be a separate package. We are still waiting on improved React APIs that, hopefully, will make this obsolete
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.
Alright, we will use it for now in the example then, and I will keep an eye in the emotion issue. Thanks!
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 just want to clarify because I was perhaps misunderstood. Those were my general thoughts on the subject but I think that it would be valuable to get this into the Emotion's repo as an experimental package or smth. The advantage of that would be that we could deprecate this package in the future with a helpful note about the migration strategy.
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.
Ah alright, that makes sense. @garronej would you like to make a contribution as you originally created it? :)
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.
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.
Also why it is needed/what it accomplishes. I see some related exposition from NextJS but don't think it is terribly clear either for novice learners: https://nextjs.org/docs/app/building-your-application/styling/css-in-js#configuring-css-in-js-in-app
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.
If it's an issue you can make a PR on this @sgoodrow-zymergen
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.
Would be great if @garronej or @Andarist can share a summary of how this work with app router.
Hello, the provider ensures that when the app is rendered on the server, the styles generated by Emotion get injected into the <head>
element.
Without the provider, Emotion defaults to its "zero config SSR mode", in which <style />
tags are injected into the body of the document, right next to the element being styled.
However, MUI is not compatible with Emotion's "zero config SSR mode". This has led to the recommended implementation with the Next Pages Router here being quite complex. Ever since MUI started utilizing Emotion, it has required the SSR advanced approach to be implemented, this provider is nothing new, it's the same old aprach implemented for the Next's App Router.
A while back, I proposed an RFC suggesting an abstraction of this process. I've also initiated a PR on the Emotion repository to discuss transferring the TSS utilities. I'm absolutely prepared to contribute to this project. But as @Andarist pointed out, it would involve introducing a new experimental Emotion module, and he's better equipped to make the necessary decisions regarding that.
examples/material-next-app-router-ts/src/components/Theme/ThemeRegistry/theme.ts
Outdated
Show resolved
Hide resolved
<React.Fragment> | ||
<CssBaseline /> | ||
<NextAppDirEmotionCacheProvider options={{ key: 'mui' }}> | ||
<ThemeProvider theme={theme}>{children}</ThemeProvider> |
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.
Agree, the CssBaseline
should be under the ThemeProvider
.
@mnajdova - cool I will address the review comments asap |
@mnajdova - The review comments have been resolved, and an additional page has been added to demonstrate how the navigation works. |
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.
Works great! Thanks a lot for the contribution @smo043 and sorry for the delay with the review. @mj12albert, @brijeshb42 we can extend this example in the future to show how RSC can be used too.
@mnajdova - I appreciate you taking the time to test and review this PR. |
@mnajdova - I don't have merge access, could you please merge this pull request? |
Yes, merging it. |
Hi I just came across this thread, can I just check if this is a solution to the problem where in the new app directory, the theme styles don't apply when you route to a new page? Update: |
I use want to use this setup with Tailwind CSS also, do I need to do any other changes to make it work? Also linter dont like these:
|
children: React.ReactNode; | ||
}; | ||
|
||
// This implementation is taken from https://github.com/garronej/tss-react/blob/main/src/next/appDir.tsx |
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.
<3
Hi @smo043, is there an option that this setup has an issue with specificity of custom styles? I am trying to override MUI styles using styled() and while the styles apply for additional CSS properties correctly, the ones that collide with MUI are always below the MUI styles in priority. So it seems as if I would have to apply I mimicked this setup and am using |
@jakubprzybylski You can change the CSS injection order like this: https://mui.com/material-ui/guides/next-js-app-router/#css-injection-order |
@mj12albert thank you for taking your time to help! Following this link, I tried to insert:
into my I've noticed that in this documentation link material-ui/examples/material-next-app-router-ts/src/components/ThemeRegistry/EmotionCache.tsx Line 47 in a18f64a
So this solution from documentation is either incorrect or outdated, because it's based on an older version of Perhaps it's an easy fix? |