examples icon indicating copy to clipboard operation
examples copied to clipboard

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

Open 3x071c opened this issue 3 years ago • 18 comments
trafficstars

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 avatar Mar 08 '22 14:03 3x071c

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

Thanks!

PJColombo avatar Mar 16 '22 18:03 PJColombo

@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.

3x071c avatar Mar 17 '22 07:03 3x071c

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.

PJColombo avatar Mar 17 '22 15:03 PJColombo

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

tavoyne avatar Apr 05 '22 13:04 tavoyne

@tavoyne Docs

3x071c avatar Apr 05 '22 14:04 3x071c

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?

tavoyne avatar Apr 05 '22 14:04 tavoyne

@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.

3x071c avatar Apr 05 '22 17:04 3x071c

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.

tavoyne avatar Apr 06 '22 08:04 tavoyne

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

machour avatar Apr 06 '22 10:04 machour

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

tavoyne avatar Apr 06 '22 12:04 tavoyne

@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 avatar Apr 06 '22 13:04 3x071c

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.

tavoyne avatar Apr 06 '22 14:04 tavoyne

@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.

3x071c avatar Apr 06 '22 14:04 3x071c

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?

AndlerRL avatar Apr 13 '22 20:04 AndlerRL

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.

ancashoria avatar Aug 02 '22 09:08 ancashoria

If someone would like to update the emotion example to mirror this chakra example that would be awesome: https://github.com/remix-run/examples/pull/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

jacob-ebey avatar Oct 20 '22 03:10 jacob-ebey

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 avatar Oct 20 '22 17:10 chaance

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

https://github.com/remix-run/examples/blob/cc3e057e18bda4476434e365c68fa971d8e86cb3/emotion/app/root.tsx#L35-L53

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

RobinClowers avatar Oct 21 '22 23:10 RobinClowers

@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 😕

ePirat avatar Nov 13 '22 10:11 ePirat

@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.

3x071c avatar Nov 14 '22 00:11 3x071c

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

machour avatar Nov 14 '22 08:11 machour

https://github.com/remix-run/examples/blob/c8833ea3c0101f9ee14911a1a8304de804dc3fb2/emotion/app/root.tsx#L52-L53

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.

mi-na-bot avatar Nov 18 '22 08:11 mi-na-bot

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

ePirat avatar Nov 18 '22 13:11 ePirat

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 https://github.com/emotion-js/emotion/issues/2554

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

jfsiii avatar Nov 18 '22 14:11 jfsiii

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

machour avatar Nov 18 '22 14:11 machour

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 😅

chaance avatar Nov 18 '22 17:11 chaance

@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 avatar Nov 18 '22 18:11 3x071c

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

3x071c avatar Nov 18 '22 18:11 3x071c

I just opened https://github.com/remix-run/examples/pull/78 to fix this issue.

RobinClowers avatar Nov 22 '22 03:11 RobinClowers

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

mi-na-bot avatar Nov 26 '22 02:11 mi-na-bot