[docs] Fix global styles leaking on different pages #25855
Conversation
This comment has been minimized.
This comment has been minimized.
|
@mnajdova The changes break the SSR output, on all the pages. I assume the extractCritical used on the server is overridden by the second RTL/LTR cache. |
|
We will need to update our Next.js examples. |
|
Found it - #22731 we needed this so that SSR will work :\ |
Yeah I noticed, not sure how to make both of them work at the same time. Looks like they are contradicting, or I am doing something wrong.. Probably the second :D |
|
@mnajdova We could, for instance, only mount the RTL cache after the hydration. In JSS I had to use a special prop to allow the nesting of multiple StyleProvider to work with SSR, for this very configuration. |
Interesting, ok I'll try this tomorrow. |
|
@oliviertassinari I believe everything should be fixed now. I was missing
Let's wait for the prod build and verify and I think it should be good to go |
docs/pages/_document.js
Outdated
|
|
||
| const { extractCritical } = createEmotionServer(cacheLtr); | ||
| const cache = createCache({ key: 'css', prepend: true }); |
There was a problem hiding this comment.
Hm, this doesn't look like creating the cache per render - unless in your setup this file is reexecuted each render. Don't you have global styles leaking between pages right now?
There was a problem hiding this comment.
Good catch! I reverted back the getCache method. Thanks :)
There was a problem hiding this comment.
👍 to @Andarist's comment, the cache is still global, it leaks: https://deploy-preview-25855--material-ui.netlify.app/components/grid/#interactive (Cell 1 is font-family Inter, our branding)
When I was cleaning up I reverted the changes for |
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
docs/pages/_document.js
Outdated
| @@ -9,8 +9,9 @@ import { pathnameToLanguage } from 'docs/src/modules/utils/helpers'; | |||
| import { themeColor } from 'docs/src/modules/components/ThemeContext'; | |||
| import createCache from '@emotion/cache'; | |||
|
|
|||
| const cache = createCache({ key: 'css', prepend: true }); | |||
| const { extractCritical } = createEmotionServer(cache); | |||
| const getCache = () => createCache({ key: 'css', prepend: true }); | |||
There was a problem hiding this comment.
This shouldn't break anything (I think), but the overall recommendation is to use custom keys when creating caches and the "css" is kinda a default one.
There was a problem hiding this comment.
Should this be identical with the one we use on the client? For example use mui on both places?
PS: I will leave this for a follow up PR 👍
There was a problem hiding this comment.
Yes, definitely it should be the same - this is used for rehydrating stuff and so on.
There was a problem hiding this comment.
It's not working <style id="emotion-server-side" data-emotion="css "></style> is empty. See https://validator.w3.org/nu/?doc=https://deploy-preview-25855--material-ui.netlify.app/components/grid/#interactive
Adding cache.compat = true; will probably do it
It was broken with 2adb0fa, once the global styles leaks were solved, this broke 😕 So basically I am at this:
I tried using Let me open additional PR that fixes the SSR and compare and see what are the changes and what we can do. I am running in circles now... Edit: opened #25863 so that we can test my theory |
|
So on https://deploy-preview-25863--material-ui.netlify.app/ (PR #25863) ssr is fixed, but the leaking is still there |
|
@mnajdova Please have a look at my latest commit. It seems to fly. |
I could still repro the leaking:
|
|
@oliviertassinari last try, I've tried to incorporate these changes together with the changes from emotion-js/emotion#2334 in #25690 I think everything works over there, but I may be missing something (I can't trust myself on this one anymore):
|
@mnajdova Oh yes correct. This time, the leak is not on the server-side, but on the client-side. We end up with:
This looks like a different issue, isn't the same as emotion-js/emotion#2158 where the old global isn't removed from the DOM when unmounted? |
@mnajdova I corroborate 👌 |
Good! I started to lose my mind over this one 😄 I'd say let's merge this one, and after upgrading emotion, we can verify again that this new scenario works. Edit: Or we can wait and merge only #25690 once we have new release of emotion |
|
@mnajdova IMHO these two issues are independent, happy with whatever order |
|
Ok, I'll merge this one and rebase #25690 |
|
🎉 |
As per emotoin's documentation, the cache should be re-created per request (see the note https://emotion.sh/docs/ssr#extractcritical). So far we've been reusing the same cache instance, which was leading to global styles leaking on different pages.
Preview: https://deploy-preview-25855--material-ui.netlify.app/components/grid/#interactive
Fix #24970