remix icon indicating copy to clipboard operation
remix copied to clipboard

[Bug]: Style tags disappear from Head during Error/Catch Boundary

Open hgeldenhuys opened this issue 3 years ago • 41 comments

Which Remix packages are impacted?

  • remix (Remix core)
  • @remix-run/react

What version of Remix are you using?

1.1.1

What version of Node are you using? Minimum supported version is 14.

16.13.0

Steps to Reproduce

Inject style tags into header in entry.server.tsx during markup generation. Example:

let markup = renderToString(
    <RemixServer context={remixContext} url={request.url} />
  );

  const html = markup.replace(
    "<head>",
    `<head>${getSSRStyles(markup, server)}`
  );

  responseHeaders.set("Content-Type", "text/html");

  return new Response("<!DOCTYPE html>" + html, {
    status: responseStatusCode,
    headers: responseHeaders,
  });

Expected Behavior

The style tags should stay intact during 404, or at least after 404, refetch the HTML from server.entry.

I'm guessing because Remix only generates links an meta tags in head dynamically, things like style and base will get overridden when Error boundary generates a new document. This is unfortunate, as initial load will contain Style tags that prevent the page from loading janky and will be lost during errors.

This prevents us from using major UI libraries like Material UI and Mantine, which generate dynamic styles based on frameworks like emotion. The CatchBoundary/Error should re-render from server-side, not client-side (rehydrate head tag from server.entry)

Actual Behavior

Error/Catch boundaries removes everything in Head tag, not just the dynamically generated head-elements provided by the route, meaning style tags get removed, that are "global" styles.

hgeldenhuys avatar Dec 18 '21 16:12 hgeldenhuys

I would also add in that script tags from the end of the body are removed too so remix context gets lost other among other things.

pixelstew avatar Dec 22 '21 23:12 pixelstew

So far I have ascertained a few things relating to this issue.

IMPORTANT

Navigating in browser to a non-existing url does NOT reproduce this issue, entry.server runs and produces correct results.

INSTEAD

Clicking a link in app to a non-existing url renders catchBoundary without running entry.sever and thus styles generated in this step are not generated and passed to entry.client for insertion.

As well as losing any user generated style tags from the head the Remix Context does not get loaded in catchBoundary. Then when user clicks 'back' it is also missing from the previous (genuine) route too. Instead there is an empty <script></script> where context is usually generated.

pixelstew avatar Dec 23 '21 22:12 pixelstew

Just to add you can't hit the "back" button in this scenario either, it will throw another error around appendingChild as the loader will run but have no context anymore (I assume)

image

TheRealFlyingCoder avatar Jan 04 '22 06:01 TheRealFlyingCoder

Well, the issue is that the top-most (root.tsx) error & catch boundary - if default unchanged - re-renders the whole document, including head. The JSX does not including the styles injected directly via dom by for example emotion.

I think it could be partially worked around using some layout root with error and catch boundaries one level underneath the root.tsx. Still, if there is a problem in this layout route, it will fall back to root.tsx, but at least not for every error and you are not required to unnaturally have boundaries all over the place.

Alternative, I did some research, would be to re-inject all the styles in effect. However the material UI style engine at first glance does not seem to give access for the underlying emotion sheet instance (I did only a glance check). Also, re-injecting as effect might cause flickr after initial boundary render

akomm avatar Jan 25 '22 07:01 akomm

I'm having a very similar issues (although using styled-components). Any catch/error boundaries that I create in the root.tsx required me to re-render the whole document.

As @akomm said, it would be great to have some "layout" middle piece that only re-renders a piece in the body and not the whole document.

kevinbailey25 avatar Feb 11 '22 20:02 kevinbailey25

Or at least not the Head-tag, since components live in the body only, but the route could at the very least inherit the same head

hgeldenhuys avatar Feb 12 '22 14:02 hgeldenhuys

The styled-components official example has this error too when adding error/catch boundaries.

So far I have ascertained a few things relating to this issue.

IMPORTANT

Navigating in browser to a non-existing url does NOT reproduce this issue, entry.server runs and produces correct results.

INSTEAD

Clicking a link in app to a non-existing url renders catchBoundary without running entry.sever and thus styles generated in this step are not generated and passed to entry.client for insertion.

As well as losing any user generated style tags from the head the Remix Context does not get loaded in catchBoundary. Then when user clicks 'back' it is also missing from the previous (genuine) route too. Instead there is an empty <script></script> where context is usually generated.

In addition to this, I found that navigating to a non-existing url using a normal anchor tag (<a href="/foo">Go to</a>) won't break the styling. I asume that this is an equivalent to refreshing the website but with a new route, which will be the same to: Navigating in browser to a non-existing url does NOT reproduce this issue, entry.server runs and produces correct results.

hazzo avatar Apr 12 '22 10:04 hazzo

Any idea of how to resolve this? This is one of the last pieces preventing us from converting our apps at work to Remix.

kevinbailey25 avatar May 05 '22 22:05 kevinbailey25

@kevinbailey25

Add pathless layout route Add ErrorBoundary and CatchBoundary in this pathless-layout-routes

Add the pathless route directly descending your root route.

Now the issue should only appear, if you have an error in your pathless route, but not any route beneath it.

akomm avatar May 06 '22 14:05 akomm

Thank you @akomm! This should do exactly what I need. I knew about the pathless routes in react-router but didn't know about the double underscore convention in remix.

kevinbailey25 avatar May 06 '22 15:05 kevinbailey25

I'm experiencing a similar issue, and I was able to resolve component errors by doing what @akomm mentioned. However, even with using a CatchBoundary in the pathless route, 404s are still being unhandled, I'm assuming since they are not originating inside the pathless route, any way to handle that?

Podskio avatar May 07 '22 17:05 Podskio

@jpod32 I just tested manually throwing an 404 response and the catch boundary works in a regular route. I don't have yet the possibility to make a test on a pathless route. If you want me to check into it, you could create a reproduction sandbox. It is possible that catch boundary does not work when the routing fails, hence my manually thrown 404 response is not the same. Or it might be only issue with pathless routes. Or maybe you'v overlooked something?

akomm avatar May 09 '22 08:05 akomm

My issue is with 404s that aren't thrown manually, which can just be reproduced by entering an invalid url. From my attempts, this doesn't go to the catch boundary inside the pathless route.

Podskio avatar May 11 '22 22:05 Podskio

I'm also experiencing this, and can second @jpod32's experience. I tried the pathless route solution and had the same issue for 404s from invalid URLs.

Has anyone else found a workaround to this yet?

oscarnewman avatar May 26 '22 20:05 oscarnewman

TL;DR: talked to Ryan Florence about it... so he at least knows it's a pain point for those of us trying to migrate to Remix, but I have no perfect solution.

I lucked out at Remix Conf to have Ryan Florence sit at my table for lunch during the workshop. He asked everyone what's one thing that you wished Remix had and I brought up a lot of the styling pains I've had while trying to migrate.

Remix really wants to give you full control of your network tab. Hence why they want you to set the url for your css in the links function. Which I totally get. If I could go back in time when we started this project at work, I would have picked tailwind, but at the time we picked styled-components. I think a lot of developers are used to writing css that sits along side their components and magically makes it into their app. My angular skills are a bit rusty now, but I think even with that framework you would specify a css file for each component if needed.

Then if you jumped into styled-components... it was... write your css that goes with that component.

Things get more complicated when you've written a component library that's a separate package that expects styled-components to just work.

I get why Remix went the way they did, but we can't re-write the whole app to get away from styled-components... I just can't convince the higher ups right now that it's worth it.

Hopefully they can help get some happy paths for us using css-in-js libs. :/

kevinbailey25 avatar May 26 '22 22:05 kevinbailey25

Any progress on this bug, it is the one thing preventing us to use Remix with Mantine. Please give us feedback so at least we understand why fixing this issue is not easy. Thank you for your awesome work!

absamo avatar Jun 01 '22 05:06 absamo

@absamo its not exactly a bug, since it behaves as it should, the whole document is re-rendered and anything injected directly into the DOM, like normally for react, disappears.

But I agree this is an important to solve issue. Some statement would be nice, even if its "we can't for this reason" or "it would mean this and that". Maybe someone get an idea based on this how to still make it better, if a 100% perfect solution is not possible.

Oh and its clear, one of the options, that the devs probably don't want to use is removing the control over the document itself in the way we have it yet. Currently its kind of first-class, as any other components.

akomm avatar Jun 01 '22 08:06 akomm

Any progress on this bug, it is the one thing preventing us to use Remix with Mantine. Please give us feedback so at least we understand why fixing this issue is not easy. Thank you for your awesome work!

It‘s the same for me. I have tried Remix, loved it immediately for its data fetching model and then I’ve noticed the bug. It was heartbreaking experience 😉.

kwiat1990 avatar Jun 04 '22 07:06 kwiat1990

Had the same problem when trying to use Mantine. I was able to work around it by reapplying the styles on the client using emotion cache, it should also work for other libraries that use emotion but will probably need another context for server styles.

Made a small repo with an example, let me know if it helps @kwiat1990 @absamo

correiarmjoao avatar Jun 04 '22 16:06 correiarmjoao

Had the same problem when trying to use Mantine. I was able to work around it by reapplying the styles on the client using emotion cache, it should also work for other libraries that use emotion but will probably need another context for server styles.

Made a small repo with an example, let me know if it helps @kwiat1990 @absamo

Wow, thanks for the repo. I forked it and I will definitely try it after my vacations!

kwiat1990 avatar Jun 04 '22 19:06 kwiat1990

Yes, basically this as I'v mentioned:

Alternative, I did some research, would be to re-inject all the styles in effect. However the material UI style engine at first glance does not seem to give access for the underlying emotion sheet instance (I did only a glance check). Also, re-injecting as effect might cause flickr after initial boundary render

But sometimes you use it via other libraries, which don't expose the instances of cache they'v created.

akomm avatar Jun 06 '22 21:06 akomm

Had the same problem when trying to use Mantine. I was able to work around it by reapplying the styles on the client using emotion cache, it should also work for other libraries that use emotion but will probably need another context for server styles.

Made a small repo with an example, let me know if it helps @kwiat1990 @absamo

Thanks @correiarmjoao

I can confirm this works. Simply pay attention to the following files when making changes to your own project:

After updating my project accordingly, the problem has been solved.

There is though the problem of Flash Of Unstyled Content in Mantine that occurs when navigating to a 404 page. It's not a big deal for me but something you should be aware of. Other supported frameworks(Gatsby, Next) and Chakra UI users are also facing the same issue.

brandiqa avatar Jun 08 '22 11:06 brandiqa

@brandiqa was the FOUC not because of Next or /and using npm as package manager? I had myself that problem, which I could solve using Mantine’s Next starter template.

kwiat1990 avatar Jun 08 '22 12:06 kwiat1990

@brandiqa was the FOUC not because of Next or /and using npm as package manager? I had myself that problem, which I could solve using Mantine’s Next starter template.

I am currently using yarn. Navigating between existing routes works fine. FOUC only appears when the 404 error page is generated. How did you solve the problem?

brandiqa avatar Jun 08 '22 13:06 brandiqa

With Remix I didn’t try it yet. But in my Next project I needed to switch to yarn and downgrade Next to version 12.1.4 to get rid of FOUC.

kwiat1990 avatar Jun 08 '22 14:06 kwiat1990

Interesting... I just checked the deployed/production version and there's no FOUC. I guess it's a non-issue after all since it only happens when running remix in dev mode.

brandiqa avatar Jun 08 '22 14:06 brandiqa

Had the same problem when trying to use Mantine. I was able to work around it by reapplying the styles on the client using emotion cache, it should also work for other libraries that use emotion but will probably need another context for server styles.

Made a small repo with an example, let me know if it helps @kwiat1990 @absamo

Awesome, nice work around. Thanks for sharing it.

absamo avatar Jun 08 '22 21:06 absamo

~~@correiarmjoao I've tried it and it seems to be working but I've found odd behaviour: if some route doesn't exist all Mantine's styles would be correctly applied but not those for body. In my case<body> gets default margin of 8px, which normally is overridden by Mantines global reset.~~

@correiarmjoao, i have forked your repo and built based on it and luckily I didn't experience FOUC anymore. Thanks!

kwiat1990 avatar Jun 12 '22 11:06 kwiat1990

same issue here +1

liho00 avatar Jun 25 '22 05:06 liho00

@correiarmjoao do you have an idea how it can be done for the new Mantine 5.0? There was change to cache and this solution doesn't work anymore. I have also played with it a little bit but I can't really get it working.

kwiat1990 avatar Jul 04 '22 08:07 kwiat1990