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] Next.js v13 app router with Material UI #37315

Merged
merged 9 commits into from
Jun 22, 2023
Merged

Conversation

smo043
Copy link
Contributor

@smo043 smo043 commented May 18, 2023

@smo043 smo043 changed the title feat: next js 13 app router with mui5 [core] next js 13 app router with mui5 May 18, 2023
@mui-bot
Copy link

mui-bot commented May 18, 2023

Netlify deploy preview

https://deploy-preview-37315--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against d4d0f72

@michaldudak michaldudak added the examples Relating to /examples label May 18, 2023
@michaldudak michaldudak requested a review from mnajdova May 18, 2023 07:02
@michaldudak michaldudak changed the title [core] next js 13 app router with mui5 [examples] next js 13 app router with mui5 May 18, 2023
@smo043
Copy link
Contributor Author

smo043 commented May 19, 2023

This change will make it easier for people who are trying to use Next App Router with Material UI v5.

@smo043 smo043 requested a review from MartinXPN May 20, 2023 15:05
@smo043 smo043 mentioned this pull request May 20, 2023
22 tasks
'use client';

import * as React from 'react';
import { Container, Box, Typography } from '@mui/material';
Copy link
Member

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:

Suggested change
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
Copy link
Member

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.

Suggested change
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

Copy link
Contributor Author

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?

Comment on lines +5 to +9
modularizeImports: {
'@mui/icons-material': {
transform: '@mui/icons-material/{{member}}',
},
},
Copy link
Member

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.

Comment on lines 10 to 12
experimental: {
typedRoutes: true,
},
Copy link
Member

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?

Suggested change
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';
Copy link
Member

@oliviertassinari oliviertassinari May 21, 2023

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 oliviertassinari changed the title [examples] next js 13 app router with mui5 [examples] next js 13 app router with Material UI v5 May 21, 2023
@oliviertassinari oliviertassinari changed the title [examples] next js 13 app router with Material UI v5 [examples] Next.js v13 app router with Material UI v5 May 21, 2023
@smo043
Copy link
Contributor Author

smo043 commented May 22, 2023

@oliviertassinari - thank you, I will work on your review comments.

@smo043 smo043 requested a review from oliviertassinari May 22, 2023 17:19
export default function RootLayout({ children }: { children: React.ReactNode }) {
return (
<html lang="en">
<body>

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@smo043 smo043 May 23, 2023

Choose a reason for hiding this comment

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

with next/fonts: Next.js automatically generates a styles file and inserts the fonts reference to it.

font_styles font

@smo043 smo043 requested a review from MartinXPN May 23, 2023 10:09
Copy link

@MartinXPN MartinXPN left a 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

coulgreer added a commit to coulgreer/coulgreer.me that referenced this pull request May 23, 2023
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
@smo043
Copy link
Contributor Author

smo043 commented May 25, 2023

@oliviertassinari @mnajdova - can we merge this pr?

<React.Fragment>
<CssBaseline />
<NextAppDirEmotionCacheProvider options={{ key: 'mui' }}>
<ThemeProvider theme={theme}>{children}</ThemeProvider>

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!

Copy link

@kernelwhisperer kernelwhisperer May 30, 2023

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.

Copy link
Contributor Author

@smo043 smo043 Jun 1, 2023

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?

Copy link
Member

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.

@taschetto
Copy link

I may be missing something, but this approach uses use client for every mui component, so we are not leveraging SSR here.

@joeydqyuan
Copy link

I may be missing something, but this approach uses use client for every mui component, so we are not leveraging SSR here.

I have the same question. Could you please help me answer it? thanks

@MartinXPN
Copy link

@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 pages directory) but you'll also get the benefit of interactivity with JavaScript on the client.

@garronej

This comment was marked as off-topic.

@mnajdova
Copy link
Member

I understand that some people might not be keen on adding an extra dependency to their project if they are not using tss-react. However, this tooling is not specifically tied to tss-react. In my opinion, it could be provided either by the Emotion package or MUI.

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 :)

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 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor

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

Copy link
Member

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!

Copy link
Contributor

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.

Copy link
Member

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? :)

Copy link
Member

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.

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

Copy link
Contributor

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

Copy link
Contributor

@garronej garronej Jun 24, 2023

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.

@mnajdova @siriwatknp @sgoodrow-zymergen

<React.Fragment>
<CssBaseline />
<NextAppDirEmotionCacheProvider options={{ key: 'mui' }}>
<ThemeProvider theme={theme}>{children}</ThemeProvider>
Copy link
Member

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.

@smo043
Copy link
Contributor Author

smo043 commented Jun 21, 2023

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.

@mnajdova - cool I will address the review comments asap

@smo043
Copy link
Contributor Author

smo043 commented Jun 21, 2023

@mnajdova - The review comments have been resolved, and an additional page has been added to demonstrate how the navigation works.

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.

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.

@smo043
Copy link
Contributor Author

smo043 commented Jun 22, 2023

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.

@smo043
Copy link
Contributor Author

smo043 commented Jun 22, 2023

@mnajdova - I don't have merge access, could you please merge this pull request?

@mnajdova
Copy link
Member

@mnajdova - I don't have merge access, could you please merge this pull request?

Yes, merging it.

@mnajdova mnajdova merged commit c30ce7f into mui:master Jun 22, 2023
@mnajdova mnajdova changed the title [examples] Next.js v13 app router with Material UI v5 [examples] Next.js v13 app router with Material UI Jun 22, 2023
@marcusleeeugene
Copy link

marcusleeeugene commented Jun 22, 2023

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:
Just tried, and it fixes the issue, thank you! :)

@c0d3x
Copy link

c0d3x commented Jun 22, 2023

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:

./src/components/Theme/ThemeRegistry/EmotionCache.tsx
31:15  Error: 'cache' is already declared in the upper scope on line 29 column 14.  no-shadow
43:15  Error: 'flush' is already declared in the upper scope on line 29 column 21.  no-shadow
59:23  Warning: Generic Object Injection Sink  security/detect-object-injection

children: React.ReactNode;
};

// This implementation is taken from https://github.com/garronej/tss-react/blob/main/src/next/appDir.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@jakubprzybylski
Copy link

jakubprzybylski commented Aug 1, 2023

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 !important for everything, which isn't the greatest perspective. :)

I mimicked this setup and am using MUI 5.14.0 with Next 13.4.10.

@mj12albert
Copy link
Member

@jakubprzybylski You can change the CSS injection order like this: https://mui.com/material-ui/guides/next-js-app-router/#css-injection-order

@jakubprzybylski
Copy link

jakubprzybylski commented Aug 2, 2023

@mj12albert thank you for taking your time to help! Following this link, I tried to insert:

__html: options.prepend ? `@layer emotion {${styles}}` : styles

into my EmotionCache.tsx, however the styling looks even worse now with MUI elements losing their original padding for instance.

I've noticed that in this documentation link useServerInsertedHTML()'s function body is way shorter and different than the one found in EmotionCache.tsx stored in master or 3.4.0:

So this solution from documentation is either incorrect or outdated, because it's based on an older version of EmotionCache.tsx, I think?

Perhaps it's an easy fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Relating to /examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.