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.

Sign up for GitHub

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

Jump to bottom

"Advanced approach" SSR with Next broken with react-dom 18 #2725

Closed
garronej opened this issue Apr 16, 2022 · 8 comments
Closed

"Advanced approach" SSR with Next broken with react-dom 18 #2725

garronej opened this issue Apr 16, 2022 · 8 comments
Assignees
@Andarist
Labels
bug needs triage

Comments

@garronej
Copy link
Contributor

garronej commented Apr 16, 2022 •
edited

Hi,
Again, thank you for your work on Emotion, it's a fantastic library.

UPDATE: Scroll down to this message, it better describe the issue.
@garronej garronej mentioned this issue Apr 16, 2022
Closed
@srmagura
Copy link
Member

Emotion inserts styles differently in React 18 to be compatible with React's new SSR streaming capabilities.

Could you explain what the expected behavior vs. actual behavior is for Emotion in the context of React 18? Ideally you would also provide a minimal reproducible example.

BTW, I tried running your project on Windows but could not because of the Unix-specific scripts.

garronej added a commit to garronej/tss-react that referenced this issue Apr 17, 2022
@garronej
Make test app runnable on Windows #78 emotion-js/emotion#2725
cd8fef9
garronej added a commit to garronej/tss-react that referenced this issue Apr 17, 2022
@garronej
Make test app runnable on Windows #78 emotion-js/emotion#2725
3617737
@garronej
Copy link
Contributor Author

garronej commented Apr 18, 2022 •
edited

Hi @srmagura thank you for your answer,

As I am sure you know, MUI is using emotion.
With MUI, 0-config SSR isn't supported, to enable SSR, MUI users are expected to explicitly provide an emotion cache and manually insert the <style> tags in the <head> as described in the "advanced approach" section of the emotion documentation.
You can find here a canonical setup for MUI + Next.js. This approach no longer works with react 18.

Reproduce

I created a minimal repo to exhibit what is no longer working as it used to.
Code: https://github.com/garronej/ssr-react-18-next

Fist disable JavaScript

git clone https://github.com/garronej/ssr-react-18-next
cd ssr-react-18-next
yarn
yarn build
yarn start
#heads over to http://localhost:3000

You'll see:
image

Now, if you downgrade to react-dom 17.0.2 you'll get:

image

What's happening

You can see this <style> tag inserted at the end of the <head> should take priority over color: read generated by css here.

image

The red string tag is not generated with react-dom 17 and causes the green style to be overwritten.

My 2 sence

If you run the app with yarn dev the bug happens only the very first render but we can see that with react-dom 18 unlike with react-dom 17 the _app.tsx is run twice and the first time it's before getInitialProps() of our custom _document.tsx has been called.

Screen.Recording.2022-04-18.at.02.22.21.mov

In the hope that a solution can be found,
Best

@garronej garronej mentioned this issue Apr 18, 2022
Closed
2 tasks
@srmagura
Copy link
Member

@garronej Thank you so much, that really clears it up.

The "Advanced Approach" docs seem to be incomplete or incorrect for React 18 at the moment. Someone will need to do some testing with Next and MUI and either fix or add to that section in our docs.

Apart from the aforementioned documentation issue, I don't see anything Emotion is doing that is "wrong". My current feeling is that MUI is not compatible with the new streaming SSR, and that that is the underlying issue.

After reading the relevant code in Emotion (which is quite complex), I think Emotion will always insert <style> tags into the <body> when using React 18, even if the Advanced Approach is used and SSR streaming is not used. (I could be reading the code wrong though.) In React 17 when using the Advanced Approach, Emotion does not directly insert <style> tags during SSR, but our docs recommend you manually place them in the <head>.

Maybe we need to insert a toggle into Emotion to allow users to revert back to the React 17 behavior even when using React 18? I am thinking of this as a stopgap until MUI can fully support SSR streaming.

A lot of the discussion on mui/material-ui#24109 is likely relevant to this.

@Andarist Andarist mentioned this issue Apr 19, 2022
Closed
@Andarist
Copy link
Member

I've created a Next.js issue about this here.

As a workaround you set clientSideEmotionCache.compat = true here. However, I think that the setup used in this repro case is slightly weird - you probably shouldn't use CacheProvider in your MyApp. When combined with Document.getInitialProps this will result in wrapping the whole thing twice with the CacheProvider and you don't pass the "outer" cache to the inner CacheProvider - I would suspect this leading to some unexpected results.

@Andarist
Copy link
Member

The created issue was closed by this PR so I will assume that this should get fixed with the next Next release.

@Andarist Andarist mentioned this issue Apr 29, 2022
Closed
@garronej
Copy link
Contributor Author

garronej commented May 3, 2022

Hi,
Next 12.1.6 just released.
I confirm that it solves the issue.

@himadrinath
Copy link

looks like the problem arrived again in canary release.
can any one confirm that

@garronej
Copy link
Contributor Author

Hi @himadrinath,

I just tested with the current canary version,
This specific issues is fixed, there is no regression.

However, there are other issues, see the warning of tss-react:

🚨 WARNING: Next.js + React 18 -> Broken SSR.
This is not an issue specific to tss-react it affects MUI as a whole.
More specifically, there is currently no working configuration for Next.js + React 18 + Emotion advanced approach (which is the only strategy supported by MUI).
Follow the advancement of the related issues on the Next.js repo, on the MUI repo and on the TSS repo.

@srmagura srmagura mentioned this issue May 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Andarist Andarist

Labels
bug needs triage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@srmagura @garronej @Andarist @himadrinath