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

hgeldenhuys commented 2 years ago

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.

pixelstew commented 2 years ago

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 commented 2 years ago

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.

TheRealFlyingCoder commented 2 years ago

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)

akomm commented 2 years ago

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

kevinbailey25 commented 2 years ago

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.

hgeldenhuys commented 2 years ago

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

hazzo commented 2 years ago

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.

kevinbailey25 commented 2 years ago

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

akomm commented 2 years ago

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

kevinbailey25 commented 2 years ago

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.

Podskio commented 2 years ago

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?

akomm commented 2 years ago

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

Podskio commented 2 years ago

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.

oscarnewman commented 2 years ago

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?

kevinbailey25 commented 2 years ago

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. :/

absamo commented 2 years ago

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!

akomm commented 2 years ago

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

kwiat1990 commented 2 years ago

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

correiarmjoao commented 2 years ago

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

kwiat1990 commented 2 years ago

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!

akomm commented 2 years ago

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.

brandiqa commented 2 years ago

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.

kwiat1990 commented 2 years ago

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

brandiqa commented 2 years ago

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

kwiat1990 commented 2 years ago

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.

brandiqa commented 2 years ago

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.

absamo commented 2 years ago

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.

kwiat1990 commented 2 years ago

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

liho00 commented 2 years ago

same issue here +1

kwiat1990 commented 2 years ago

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

dfyz011 commented 2 years ago

I have the same problem with styled-components. I like ideas of @akomm and @hgeldenhuys to have mechanism that allows us to copy head to newly rendered document or re-render only body.

correiarmjoao commented 2 years ago

@kwiat1990 I had the same problem today, I updated the repo to work with version 5

muco-rolle commented 2 years ago

Hello everyone,

I'm facing the same issue with Chakra UI was there anyone who was able to find the fix with Chakra UI?

kevinbailey25 commented 2 years ago

@muco-rolle

I have A solution. It's not ideal but it seems to work pretty well. (my solution is using styled-components, but I assume it should work with any css-in-js lib that adds styles to the of the page)

routes/root.tsx <-- your root should have your <head> and <body> tags. You also shouldn't have any loader or action in this file. It also doesn't hurt to set export const unstable_shouldReload = () => false. The goal here is to make our root never re-render. Re-rendering is what causes all the styles to be removed (from my understanding). My root is pretty bare bones, it doesn't have much in here.

Now for the magic part. Pathless Layout Routes

Create a new pathless layout route that will be like your new "root". I call mine __main or even __root. Do what makes sense to you. routes/__main <-- folder containing all your normal route modules routes/__main.tsx <-- this is almost like your new root route. My route component, it literally just a <Outlet />.

The other important parts in your routes/__main.tsx you should have a CatchBoundary and ErrorBoundary.

I also like to add a catch all route that I can force a 404. So my CatchBoundary in my main and catch it. `routes/main/$.tsx`.

I hope this all makes sense, but this is what I've been doing and so far I haven't had any issues. I'm hoping to make a sample project showing this, but I haven't gotten around to it yet.

muco-rolle commented 2 years ago

Thanks, @kevinbailey25 this definitely helped me understand a lot of things.

Following your descriptions, I was able to make a working example I would appreciate very much your review

kevinbailey25 commented 2 years ago

Thanks, @kevinbailey25 this definitely helped me understand a lot of things.

Following your descriptions, I was able to make a working example I would appreciate very much your review

Hey @muco-rolle, I took a look at your working example, at it's core, this is just like my solution and I'm glad it working out so far for you.

I only have a few suggestions that I hope you find helpful.

In your catch all route $.tsx I would suggest putting default export blank component in there. This is how remix will know if your route is a resource route or a regular one (without the default export, it thinks it's a resource route). The reason for this, is when you use a Link Component to link to a non-existing page (like <Link to='/not-found'>), client side routing happens, tries to fetch data from your $.tsx loader, gets a 404 and knows to show your CatchBoundary.

If you do a hard-refresh of that same /not-found page or even just type /whatever-you-want (any url to non-existing page) you'll see you get a very different result when the page is first server-rendered. Remix thinks this is just a resource route, so it'll server render it without any of the parent routes. If you place an empty default export, this will then help remix know to render all of the parents routes, thus being able to use your CatchBoundary.

export function loader(){
  throw new Response("Not Found", { status: 404 }
}

export default function NotFound(){
  return <></>
}

If you have components, providers, etc that don't change, you can put those in your routes/root.tsx. For example your <ChakraProvider>. That way you don't have to put that in your CatchBoundary, ErrorBoundary and default export of your __main.tsx. Some of my apps have my navigation shell in my routes/root.tsx file. That way my Catch pages and Errors can still have the main navigation available to my users. But if that layout changes (maybe due to a user logged in or something), you'll want that probably in your __main.tsx. If your theme for the ChakraProvider doesn't ever change, then you can safely move it to the routes/root.tsx.
I highly suggest reading the docs about Module Constraints specifically about No Module Side Effects. In your __main.tsx around line 18, you have a const theme = extendTheme({}). This causes a module side effect. If you don't know what that is (I didn't either until yesterday), the remix docss that I linked to a much better job explaining them than I could. Although what you current have will work and likely be totally fine, it's good to understand module side effects because they can totally cause you issues (again, read that section, it's good info).
I'm not very familiar with Chakra UI, but it looks it uses Emotion under the hood to do it's styling. I suggest taking a look at the Emotion example that remix has. This will help you get things set up so styles will also be server rendered. Your example now only does client side rendering (Emotions default), which will potentially show you a flash of unstyled content until emotions client side styling can happen.

Hope this helps! Happy coding

muco-rolle commented 2 years ago

I couldn't ask for a better explanation and guidance than that. Thanks @kevinbailey25

jgentes commented 2 years ago

I'd like to reiterate the solution that @kevinbailey25 provided with what I discovered:

Adding ErrorBoundary and CatchBoundary to root will not work if you use any form of styling (~ everyone?)

So the fix is to create a __boundary which is used to export these functions using a file structure of:

app
├── root.tsx
└── routes
    ├── $.tsx // to catch 404 errors
    ├── __boundary.tsx 
    ├── __boundary
    │   ├── customers.tsx
    │   └── .. other pages within your layout

Root must contain your page layout with the <Outlet /> component, and the __boundary should only export <Outlet />. If any other components are placed inside __boundary they will render twice - once when root is rendered, then again when picked up by the pathless (__) route.

With this approach everything works as expected, although a) this should be clarified on the Routing page or better yet it would be great if b) Remix could create a virtual boundary within root so that the ErrorBoundary and CatchBoundary could be placed there instead of needing this pathless workaround.

kevinbailey25 commented 2 years ago

@jgentes thanks for the reiteration of my solution, that's a much easier explanation of this work around than what I said. I do want to point out 2 things

I believe this work around is only needed for projects that use any kind of CSS-in-JS library for styling (aka any javascript styling library that injects <style> tags into the <head> or <body>). If you're using css files (whether plain, or generated from things like Tailwind, PostCSS, etc) and export them via the Route Module links exports you won't need to do this work around. I've got several projects that use Tailwind or css files that work perfectly fine.

Make sure your $.tsx route (to catch 404 errors) is inside your __boundary folder. Otherwise it'll try to render the 404 page using the CatchBoundary of your root.tsx instead of the one in your __boundary.tsx.

app
├── root.tsx
└── routes
 ├── __boundary.tsx 
 └── __boundary
     ├── customers.tsx
     ├── .. other pages within your layout  
     └── $.tsx // to catch 404 errors

Hope this helps people!

jgentes commented 2 years ago

Thanks @kevinbailey25 I've updated my comment with the $.tsx moved into __boundary

kevinbailey25 commented 2 years ago

Just want to throw out a GOTCHA that kicked my butt with all of this last week.

TL;DR: stay on React 17 for now.

All of my work arounds I've listed previously is to prevent the <head> tag from being rehydrated/replaced because you'll lose all of your styles.

Insert React 18.2 and anyone that has a browser plugin that inserts anything into the page: https://github.com/facebook/react/issues/24430

The way React 18.2 currently hydrates the page, if anything that was generated from the server doesn't match what React wants to hydrate, React throws everything out and re-renders the document. Which in remix includes throwing out the head and replacing it, which throws out styles.

My team worked hard making sure that what gets server rendered is the same as what gets hydrated. Even with complicated things like localization, dates in different time zones, etc. Then we discovered if you have any browser extension that injects anything into the page (LastPass for example) this causes React 18.2 to re-render everything (because it will never match what we server rendered, and we can't stop users from using plugins). Thus losing your styles.

This is also a problem with lots of automation tools that also inject scripts into the page while testing.

We feel very confident that in the future, either React will have a better solution for this, or even your CSS-in-JS libraries will have a solution to this. So we're sticking with React 17 for now, we are considering going away from CSS-in-JS but we've got multiple years of component libraries and other products that are all built with CSS-in-JS, so not an easy switch by any means.

Hope this helps everyone in the meantime.

muco-rolle commented 2 years ago

I'd like to reiterate the solution that @kevinbailey25 provided with what I discovered:

Adding ErrorBoundary and CatchBoundary to root will not work if you use any form of styling (~ everyone?)

So the fix is to create a __boundary which is used to export these functions using a file structure of:
app
├── root.tsx
└── routes
    ├── $.tsx // to catch 404 errors
    ├── __boundary.tsx 
    ├── __boundary
    │   ├── customers.tsx
    │   └── .. other pages within your layout
Root must contain your page layout with the <Outlet /> component, and the __boundary should only export <Outlet />. If any other components are placed inside __boundary they will render twice - once when root is rendered, then again when picked up by the pathless (__) route.

With this approach everything works as expected, although a) this should be clarified on the Routing page or better yet it would be great if b) Remix could create a virtual boundary within root so that the ErrorBoundary and CatchBoundary could be placed there instead of needing this pathless workaround.

Can you please share an example repo where you implemented this?

kevinbailey25 commented 2 years ago

Here's an example repo that I've tossed together using styled-components. https://github.com/kevinbailey25/remix-styled-components-example

Unfortunately all of my real-world projects are for work and aren't public.

jgentes commented 2 years ago

Can you please share an example repo where you implemented this?

https://github.com/jgentes/mixpoint

jca41 commented 2 years ago

I'm also seeing a flash of unstyled content with normal css.

Currently i'm developing an app and i have a <Link /> to a bunch of missing pages. I know this is not the "normal setup" as you're not really meant to have missing links within your app!

Repro:

Open in preview mode
If you disable cache in devTools you might be able to see the white flash for a split second.

https://stackblitz.com/edit/node-5plrzf?file=app%2Froot.tsx

kevinbailey25 commented 2 years ago

I'm also seeing a flash of unstyled content with normal css.

Currently i'm developing an app and i have a <Link /> to a bunch of missing pages. I know this is not the "normal setup" as you're not really meant to have missing links within your app!

Repro:

Open in preview mode

If you disable cache in devTools you might be able to see the white flash for a split second.

https://stackblitz.com/edit/node-5plrzf?file=app%2Froot.tsx

This is pretty standard to how the web works in general. This is not unique to remix. With how you've set up your root.tsx you can simulate the same effect using 2 static html pages... one for your root and one for your 404 page (catch boundary). If you are telling your browser to disable the cache, then it will have to load the css file for each page.

jca41 commented 2 years ago

I'm also seeing a flash of unstyled content with normal css. Currently i'm developing an app and i have a <Link /> to a bunch of missing pages. I know this is not the "normal setup" as you're not really meant to have missing links within your app! Repro:

Open in preview mode

If you disable cache in devTools you might be able to see the white flash for a split second.

https://stackblitz.com/edit/node-5plrzf?file=app%2Froot.tsx

This is pretty standard to how the web works in general. This is not unique to remix. With how you've set up your root.tsx you can simulate the same effect using 2 static html pages... one for your root and one for your 404 page (catch boundary). If you are telling your browser to disable the cache, then it will have to load the css file for each page.

The cache off suggestion was just to make the issue occur frequently.

Does this mean it's probably better to keep the top level CatchBoundary on a pathless layout below root to avoid the removal and re-insertion of the css file?

kevinbailey25 commented 2 years ago

@jca41 if you want to avoid that, then yes having higher level Catch/Error Boundary will help. Typically I have multiple levels of Catch/Error Boundaries in my apps. I usually want to try to keep as much navigation and other elements on the page so if something does go wrong, the user still can navigate to other sections of the app easily.

muco-rolle commented 2 years ago

@kevinbailey25 have you tried solutions like Vanilla Extract? would you recommend it if it's a project that is in the early stage where migrating to it would not hurt?

machour commented 2 years ago

@muco-rolle should work fine, we have an example in the examples repo. Not the ideal implementation, but it's done like tailwind so you shouldn't see the problem raised in this issue.

remix-run / remix