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

[Bug]: Client-only errors are not handled correctly on initial load. (Infinite loop!) #1678

Closed
maulerjan opened this issue Jan 27, 2022 · 15 comments

Comments

@maulerjan
Copy link

maulerjan commented Jan 27, 2022

What version of Remix are you using?

1.1.3

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

17.3.0

Steps to Reproduce

  1. Clone the simple repro case: https://github.com/maulerjan/remix-issue-1678-repro
  2. npm install & npm run dev
  3. Click on the /about link and then refresh the page

Expected Behavior

Renders ErrorBoundary.

Actual Behavior

ErrorBoundary mounting errors resulting in an infinite loop.

The server correctly renders the page with no error (SSR), but then there's the client-only error during hydration. I'm not sure what exactly happens next, but the whole thing ends up in an infinite loop of mounting ErrorBoundary, which itself throws an error, which again tries to mount the ErrorBoundary and so on.

EDIT:
If the client-only error happens in the root component (App), then it's handled correctly, so this only affects nested modules' components.

@maulerjan maulerjan added the bug Something isn't working label Jan 27, 2022
@maulerjan maulerjan changed the title [Bug]: Client-only errors are not handled correctly on initial load. (Infinite loop! [Bug]: Client-only errors are not handled correctly on initial load. (Infinite loop!) Jan 27, 2022
@dvargas92495
Copy link
Contributor

In my case, the initial bug that caused this was having a reference to process.env.VAR. This led me to Remix's environment docs which unblocked me: https://remix.run/docs/en/v1/guides/envvars#server-environment-variables

@maulerjan
Copy link
Author

@dvargas92495 Glad you fixed it. But unfortunately fixing the error doesn't fix the error handling. What if it was your customer who found the bug?

@dvargas92495
Copy link
Contributor

Right I agree completely. Just wanted to mention it in case anyone else run into something similar and have a hard time debuggine because of the infinite loop nature of the issue

@Mange
Copy link

Mange commented Sep 8, 2022

This happened to me when I was using a value imported from a .server.ts file inside of a component render, which meant that server-side everything worked but client-side it would crash because the imported value was undefined.

Took me a long time to figure out since the tab would log around 300k errors per second and filled up the RAM so I had to binary search the code to find the crashing line by commenting out things and then killing the chrome tab on every reload that the error was still active.

Example code:

import { MyEnum } from "some_api.server";

export const loader = () => ({value: MyEnum.Foo });

export default function MyPage() {
  const { value } = useLoaderFunction();
  if (value == MyEnum.Foo) { // <-- Crash here; MyEnum is undefined since the import was stripped away in client bundle.
    return "Foo!";
  } else {
    return "Other";
  }
}

@gerbyzation
Copy link

gerbyzation commented Feb 1, 2023

Same issue here with a missing environment variable in a route component. They're hard to debug as the debugger is bricked along with the tab/browser as soon as the page starts running. What does help with debugging is to enable "Pause on exeptions" (both FF as well as Chrome have such an option), which will give you control over the execution before it all freezes up.

As pointed out this seems like a bad usability issue if it occurs for users

Running remix 1.11.1 on node 16

@wub
Copy link

wub commented Feb 20, 2023

This happened to me as well. I removed the ErrorBoundary and it stopped looping infinitely, and showed me the error. This was caused by me attempting to destructure something that was undefined.

Remix 1.13.0, node 19.6.1

@gerbyzation
Copy link

@wub what version of react are you using? After some further debugging noticed I could only reproduce this on react 17

@wub
Copy link

wub commented Feb 21, 2023

Hey @gerbyzation I got this on React 18.2.0

@gerbyzation
Copy link

@wub Would you be able to make a small repro that you can share?

@littlejustinh
Copy link

I spent a good part of a day trying to figure this out, and it seems that this issue happens if you dont use the full html document like the docs suggest, and possibly made worse when the root is a fragment. I could recreate this consistently like this:

export function ErrorBoundary() { return ( <> <nav>test</nav> <p>test</p> </> ); }

If you remove either the nav, or the p tag in this example, there's no longer an issue. If you change the fragment to a div (or literally anything else) there's no longer an issue.

You can recreate by forcing an error on the client side by placing a useEffect that will fail inside of a route component:

useEffect(() => { console.log('my effect'); force.client.side.errorBoundary; }, []);

I definitely suggest following the docs, and testing if you can still recreate this if you use the sample error boundary from the docs.

@brophdawg11 brophdawg11 self-assigned this Aug 3, 2023
@brophdawg11
Copy link
Contributor

I'm currently digging into this and so far, it seems to be two-fold. When reloading /about:

  1. SSR renders fine and React tries to hydrate
    • The client side render wncounters the client-side only error in the About component during hydration
    • This bubbles to the error boundary and tries to "render" that instead
    • But then this causes a hydration issue since the SSR happy path HTML does not match the CSR ErrorBoundary vdom
  2. Somewhere in there - React seems to then instead of hydrating into the existing markup, append a sibling node after the existing markup
    • This causes another error because you cannot append 2 children to document and Remix is rendering the entire document
    • Uncaught DOMException: Failed to execute 'appendChild' on 'Node': Only one element on document allowed

To prove this, I updated the app to make Remix only responsible for a <div id="app">, and the above flows happens the same way except that it doesn't throw another error hen it renders the sibling and I get the following on a reload of /about - notice the happy path SSR content is followed by the CSR error boundary:


Screenshot 2023-08-03 at 6 05 42 PM

Not that this is "correct", but it seems as if React throws it's hands up due to some combination of the errors and tries to render a sibling node - and that enters an infinite loop.

I'm inclined to think this is not so much a Remix problem, but a problem in React when hydrating at the root - and there may not be much Remix can do to resolve it.

I'll need to see if I can come up with a minimal non-Remix reproduction to prove this though.

@brophdawg11
Copy link
Contributor

This is a tricky one. I've dug a bit more into this and can still reproduce when mounting in a div (instead of hydrating the document) in Remix, but I'm having trouble reproducing in a small manual SSR setup.

The error itself which causes the duplicate markup gets deep into the weeds of react reconciliation code but it appears the issue is not that it doesn't try to delete the invalid SSR markup - I can see that a given fiber gets marked for deletion (deleteHydratableInstance()) - but the eventual code that actually deletes doesn't ever seem to run (commitDeletion()).

The order of operations seems to be:

  1. detect error during hydration
  2. append rendered boundary next to SSR DOM content
  3. delete SSR DOM content

This may just be an invalid approach in this new world of hydrating the document (something which React now recommends) since step 2 will fail every time because document can't have multiple children.

A.long those lines, I also noticed this reproduction is on React 17 and can confirm that upgrading to React 18 and using hydrateRoot fixes the issue (both when hydrating the document and when hydrating a div), so it may just be the case that this type of issue is already fixed in React 18.

@brophdawg11
Copy link
Contributor

I managed to get a non-Remix reproduction together. It seems to be that React 17 is unable to handle hydration with an ErrorBoundary outside of the root <html> element when hydrating the entire document. React 18 fixes this issue and can recover fine. I've included details in the server.mjs files in the reproduction repository:

https://github.com/brophdawg11/react-ssr-hydration-loop

I am going to close this out because this functionality of a root level ErrorBoundary outside the html document is core to Remix's error handling so there's not really anything we can do to "fix" it. If you are running Remix + React 17 and running into this, it's recommended you upgrade to React 18 which can handle this scenario.

@brophdawg11 brophdawg11 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 21, 2023
@maulerjan
Copy link
Author

Thanks @brophdawg11 for taking the time to get to the bottom of this, much appreciated. React 18 indeed seems to not have this issue.

@zackify
Copy link

zackify commented May 23, 2024

this issue is back in react 19 beta, seems to happen 100% of the time if you render an error boundary without html and body tags. once adding those, it happens some times, with the appendChild error, but not infinite looping at least.

~~edit: it is from a variable missing when error boundary is up in my own code, so it really is just obscuring the real problem in the code, once i was finally able to find this spot, it hasnt had the appendChild error.

If there was some way to surface the real error instead of looping and having an internal error, this would help a lot~~

edit 2: i just had to trigger it more times to see appendChild come up again, it sometimes hits the error boundary correctly, other times it does not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Closed
Development

No branches or pull requests

9 participants