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

Emotion example is poorly performant, re-rendering over 50 times a second #1

Closed
3x071c opened this issue Mar 8, 2022 · 33 comments · Fixed by #78
Closed

Emotion example is poorly performant, re-rendering over 50 times a second #1

3x071c opened this issue Mar 8, 2022 · 33 comments · Fixed by #78
Assignees
Labels
needs-response We need a response from the original author about this issue/PR

Comments

@3x071c
Copy link

3x071c commented Mar 8, 2022

What version of Remix are you using?

1.2.3

Steps to Reproduce

  1. Download the example app:
$ svn export https://github.com/remix-run/remix/tree/main/examples/emotion
  1. Install dependencies
  2. Log style re-render trigger:
$ vim app/entry.client.tsx # Add `console.log("🚨 RERENDER 🚨")` in `reset()` function body
  1. Run app:
$ npm run dev
  1. Look at the console:
    Browser console screenshot (clipped)

This is not only re-applying the style tags, but also re-rendering the entire app tree over 50 times a second, which isn't good at all. See my comment below for a suggestion on how to combat this problem.

Expected Behavior

A fast app

Actual Behavior

An app that is flooded with console warnings and re-renders

@3x071c 3x071c added the bug Something isn't working label Mar 8, 2022
@3x071c 3x071c closed this as completed Mar 10, 2022
@PJColombo
Copy link

Hey @3x071c, can you share here how did you solve this issue 🤔 ?.

Thanks!

@3x071c
Copy link
Author

3x071c commented Mar 17, 2022

@PJColombo If you pass an empty dependency array to useEffect, it will only re-apply the styles and re-render the entire app when Document is (un)mounted. To prevent unnecessary re-renders (around 3 on every navigation to an error boundary), I have memorized the children of the Document component (wrapping its JSX tree in a useMemo hook, and memorizing components with React.memo). This will still however re-apply the styles immediately after the page is first mounted, causing a performance hiccup on page reloads. I wrote a custom useOnRemount hook (here) to deal with that problem (you’ll have to use a context at the very top level to store a Boolean flag indicating whether the first mount has occurred or not). I’m still working on stopping useEffect from executing three times in a row when the Document is re-mounted by Remix. Re-opening as the example remains a bad template for poorly performant apps and could be improved a lot.

@PJColombo
Copy link

PJColombo commented Mar 17, 2022

I see. Thank you for the explanation @3x071c. All these performance problems keep users from using the template and makes the styled-component app example a better CSS-in-js remix alternative in my opinion.

Let's keep the issue open until it get's solved or improved.

@chaance chaance removed the bug Something isn't working label Mar 18, 2022
@tavoyne
Copy link

tavoyne commented Apr 5, 2022

Why is all the boilerplate in the emotion example even necessary? It's just working out of the box for me.

@3x071c
Copy link
Author

3x071c commented Apr 5, 2022

@tavoyne Docs

@tavoyne
Copy link

tavoyne commented Apr 5, 2022

Yes thank you I've read the docs...

Is that suggesting that the point of this whole boilerplate is to allow nth child and similar selectors?

@3x071c
Copy link
Author

3x071c commented Apr 5, 2022

@tavoyne Using emotion in the "out-of-the-box" SSR mode doesn't require an example, while the more advanced approach does. If one prefers the secondary approach for whatever reason (more control, better support etc.), I think they should get a proper example for that, just like Next.js (et al) do in their examples.

@tavoyne
Copy link

tavoyne commented Apr 6, 2022

Thanks for clarifying that. Well, the only thing we have for now is a broken, contextless example. It could be wise to rename it emotion-advanced, and also to have an emotion-basic example. You mentioned Next.js, here's what you can read on emotion's website about it:

image

That kind of context is crucial for adoption, IMO.

Most people using emotion don't have a clue about how it works in the background.

@machour
Copy link
Collaborator

machour commented Apr 6, 2022

@tavoyne We'd be more than glad to review a PR with the changes you've mentioned!

@tavoyne
Copy link

tavoyne commented Apr 6, 2022

@machour That's what I was planning to do. I just hoped that we could discuss things beforehand.

@3x071c
Copy link
Author

3x071c commented Apr 6, 2022

@tavoyne I don't think an emotion-basic example is necessary, as there's no configuration needed whatsoever. That's why most often the advanced SSR use case is being showcased in examples, because it's way harder to get right. I also feel like your suggestion is a little off-topic to mine; I just opened this to call attention to the problem with the current inefficient emotion example that some might mistakenly copy into their own projects as-is.

@3x071c 3x071c changed the title Emotion example app updates entire tree every render (exceeding maximum update depth) Emotion example is poorly performant, re-rendering over 50 times a second Apr 6, 2022
@tavoyne
Copy link

tavoyne commented Apr 6, 2022

I'm not sure to agree. The point of examples is to help newcomers, not to confuse them. You mentioned Next.js, note that it does have a basic example (which indeed is quite trivial) but has no advanced one. About the fact that it's off topic, I apologise. I thought this was about making the emotion example better.

@3x071c
Copy link
Author

3x071c commented Apr 6, 2022

@tavoyne The basic example you linked actually does extract and inline critical CSS using "advanced" SSR techniques (definitely more simple than what the emotion docs suggest though). This is the true basic example, which as you can see is so stupidly simple (just install the dependencies and use the library) that there's no reason for one (but if you feel like it, you can of course submit a PR). This "basic" approach comes with many downsides that are not mentioned in the emotion docs and are the reason why so many users and maintainers reach for the "advanced" approach instead (see these notes by the official MUI maintainer, the chakra-ui docs (a UI framework which uses emotion) etc.). I opened this in the hope of preventing more people from worsening their own app when getting chakra-ui/mui/emotion up and running with Remix, and I think providing a full example that promotes spec compliance and other problems from arising should be the default/go-to approach in the Remix examples.

@AndlerRL
Copy link

AndlerRL commented Apr 13, 2022

I've been playing with the example for a brief time and saw this error too.

First, Emotion on SSR (as styled-components) injects all the styles after reading the requested components so the server understand what to render and finish the job. This means that the styles that we create it's being send to the server and then to the browser.

By knowing that, every time that we assign emotionCache.sheet.container we are executing funcs under the hood, because Emotion is trying to read your new styles and therefore it has to change and execute under the hood, making the main emotionCache to trigger once again and, since we need that for reading emotionCache, I thought that a guard might work.

The only instance that this is happening, it's on the client useEffect at app/root.tsx, so we take that principle in advantage and we verify if the HTMLNodeElement has new nodes/children or new siblings inside of children or node like:

// Only executed on client
    useEffect(() => {
      // avoiding unwanted rendering
      if (document.head == emotionCache.sheet.container) return
// ...

At start it will take the first requested styles and, by testing this I haven't had problems with assigning new styles, removing styles/components removing nodes from function calls and going back/forward on the browser and styles are re-rendering whenever is necessary and performance issues were gone.

I hope to not to miss something?

@ancashoria
Copy link

I agree with @tavoyne. I just wanted to see if remix works with emotion and the first thing I did was to look for this example which I consider is the ideomatic way of integrating Emotion with Remix. It has confused me for sure.

@machour machour transferred this issue from remix-run/remix Sep 16, 2022
@jacob-ebey
Copy link
Member

If someone would like to update the emotion example to mirror this chakra example that would be awesome: #35

Emotion supports SSR with streaming and we should be using their native helpers instead of trying to hack our own solutions: https://emotion.sh/docs/ssr#renderstylestonodestream

@machour machour added the help wanted Extra attention is needed label Oct 20, 2022
@chaance
Copy link
Collaborator

chaance commented Oct 20, 2022

If someone would like to update the emotion example to mirror this chakra example that would be awesome: #35

Emotion supports SSR with streaming and we should be using their native helpers instead of trying to hack our own solutions: https://emotion.sh/docs/ssr#renderstylestonodestream

I plan on knocking this out today 🙂

@chaance chaance self-assigned this Oct 20, 2022
@chaance chaance removed the help wanted Extra attention is needed label Oct 20, 2022
@RobinClowers
Copy link
Contributor

RobinClowers commented Oct 21, 2022

I ran into this same issue, everything seems to work fine without out this problematic code (including nth child selectors):

({ children, title }: DocumentProps, emotionCache) => {
const serverStyleData = useContext(ServerStyleContext);
const clientStyleData = useContext(ClientStyleContext);
// Only executed on client
useEffect(() => {
// re-link sheet container
emotionCache.sheet.container = document.head;
// re-inject tags
const tags = emotionCache.sheet.tags;
emotionCache.sheet.flush();
tags.forEach((tag) => {
(emotionCache.sheet as any)._insertTag(tag);
});
// reset cache to re-apply global styles
clientStyleData.reset();
}, [clientStyleData, emotionCache.sheet]);

The emotion SSR docs to show anything similar to this that currently, maybe they used to?

@ePirat
Copy link

ePirat commented Nov 13, 2022

@chaance Hi, did you manage to figure this out? I was just stumbling over this issue trying to use MUI with Remix and there are so many similar but not quite the same examples out there how to use Emotion with Remix, it's quite confusing 😕

@3x071c
Copy link
Author

3x071c commented Nov 14, 2022

@ePirat @jacob-ebey @chaance Just tell Remix to not take over the entire document, but rather only a child of the body (like Next.js does). This will break <head>-specific functionality of remix (i.e. adding head tags through a special named export from each page), but react-helmet does a better job at this anyway. The default behavior of Remix is a terrible practice, React explicitly warns about it. Here's my fully working Remix + Emotion + Chakra UI website.

@machour
Copy link
Collaborator

machour commented Nov 14, 2022

@3x071c hey, where does React warns about hydrating the full document? Couldn't find that reference.

@minervabot
Copy link

minervabot commented Nov 18, 2022

clientStyleData.reset();
}, [clientStyleData, emotionCache.sheet]);

The present example is in fact, not correct. It causes a re-render loop by every time updating the context state in useEffect.

@3x071c I came here looking for whether Emotion works with Remix, and have found the answer to be no. The documentation is extremely negative about css-in-js, and the example is broken. MUI, for example, is pretty popular; it is short-sighted to tell its users to go away. Personally, I am scooting back to next.js where I came from.

@machour Not sure about official docs to the effect, but using document as the React container is not common. The existence and popularity of libraries like react-helmet at least strongly support the point. Pragmatically, having a <div> as the container is much easier. Often clients want to add their own weird tracking stuff (Google Tag Manager), or some overlaid gizmo in a <script> tag, and it is nice to have space for those things to work without royally messing up the React managed DOM. This is something I did not realize about Remix right away, and again, I am scooting right back to next.js where I came from.

@ePirat
Copy link

ePirat commented Nov 18, 2022

I've managed to make it work somehow by adapting the instructions from Chakra UI.

@jfsiii
Copy link

jfsiii commented Nov 18, 2022

We're using MUI w/Emotion in Remix following this example. It works with SSR (initial response is styled without/before JS) and after the page is hydrated

At one point we needed the node_compat flag for Cloudflare Workers, but that was resolved with emotion-js/emotion#2554

That Chakra UI example looks nearly identical and is easy to follow so I'd give that a shot

@machour
Copy link
Collaborator

machour commented Nov 18, 2022

Would be lovely if someone submitted a PR to the example 🙏🏼

@chaance
Copy link
Collaborator

chaance commented Nov 18, 2022

I've been meaning to do this but it fell to the bottom of my to-do list. I'll try to get back on it next week if no one else beats me to it 😅

@3x071c
Copy link
Author

3x071c commented Nov 18, 2022

@minervabot This isn't a Next.js vs Remix thing, though it is a defaults/approach issue. It's not a Remix-specific issue, rather telling React to hydrate the entire document in general is a bad practice. Once you change the DOM root container setting up emotion (and anything based on it) is as simple as following the official emotion SSR docs. The project I linked above implements this successfully.

@3x071c
Copy link
Author

3x071c commented Nov 18, 2022

@3x071c hey, where does React warns about hydrating the full document? Couldn't find that reference.

@machour It used to be in the docs, can't find it anymore. But the warning is still thrown when you let React take over document.body: https://codepen.io/3x071c/pen/poKdKmP . This is why f.e. react-helmet doesn't take over the entire document to render <head> children, but instead instructs its users to inject it into the head (same goes for Next.js default behavior)

@RobinClowers
Copy link
Contributor

I just opened #78 to fix this issue.

@minervabot
Copy link

The way the mui example handles the double-render paradox seems a lot better: https://github.com/mui/material-ui/blob/master/examples/remix-with-typescript/app/entry.server.tsx

@github-actions github-actions bot added the needs-response We need a response from the original author about this issue/PR label Jan 25, 2023
@github-actions
Copy link
Contributor

This issue has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from issues that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 15, 2023
@RobinClowers
Copy link
Contributor

Just wanted to call out that this is still broken, and #78 does resolve the issue.

@minervabot
Copy link

@RobinClowers Emotion assumes the head element is not controlled by React, but in Remix the entire document is controlled. This is a fundamental problem which does not really have a great solution.

We evaluated Remix and decided "no thanks" for this reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-response We need a response from the original author about this issue/PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.