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.

Sign up for GitHub

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

Jump to bottom

Infinite loop on unhandle error using Layout Export with ErrorBoundary #9553

Closed
michenly opened this issue Jun 3, 2024 · 9 comments · Fixed by #9566
Closed

Infinite loop on unhandle error using Layout Export with ErrorBoundary #9553

michenly opened this issue Jun 3, 2024 · 9 comments · Fixed by #9566
Labels
bug:unverified renderer:react

Comments

@michenly
Copy link
Contributor

michenly commented Jun 3, 2024 •
edited
Loading

Reproduction

This is the positive case with no Layout, but AppShell wrap around App & ErrorBoundary

  1. Go to https://stackblitz.com/edit/remix-run-remix-jfuwjb
  2. See the error loaded in browser

This is the negative case with Layout Export

  1. Go to https://stackblitz.com/edit/remix-run-remix-jfuwjb
  2. Quit dev server
  3. Rename root-with-layout.tsx to root.tsx
  4. Open dev tool with console open
  5. Start dev server
  6. Click the link to see the error showing up in page

Following the example from https://remix.run/docs/en/main/start/tutorial to set up a base example use example here https://remix.run/docs/en/main/file-conventions/root to setup Layout Export and ErrorBoundary

This might be related to this close issue #1678 (comment) but no longer related to React version.

System Info

I can reproduce this with 
- Node 18 and Node 20
- React 18
- Remix 2.8 & 2.9

Used Package Manager

npm

Expected Behavior

That unhandle error would cause Application error in browser using Layout export

Actual Behavior

Unhandle error cause infinite loading using Layout export
This was referenced Jun 3, 2024
Merged
Merged
@brookslybrand
Copy link
Contributor

It looks like there's a note about using useLoaderData inside of Layout

useLoaderData is not permitted to be used in ErrorBoundary components because it is intended for the happy-path route rendering, and its typings have a built-in assumption that the loader ran successfully and returned something. That assumption doesn't hold in an ErrorBoundary because it could have been the loader that threw and triggered the boundary! In order to access loader data in ErrorBoundary's, you can use useRouteLoaderData which accounts for the loader data potentially being undefined.

Because your Layout component is used in both success and error flows, this same restriction holds. If you need to fork logic in your Layout depending on if it was a successful request or not, you can use useRouteLoaderData("root") and useRouteError():

So I don't think this is a bug, but I do get that the error message is a little confusing. I'm not sure if there's anything we can do to improve the error. Probably a question for @brophdawg11

@frandiox
Copy link
Contributor

frandiox commented Jun 4, 2024

So I don't think this is a bug, but I do get that the error message is a little confusing. I'm not sure if there's anything we can do to improve the error.

The same thing happens with useRouteLoaderData("root"), though, which is mentioned as something valid.

Weirdly enough, the infinite loop disappears if you pass errorElement to Await: <Await resolve={...} errorElement={'something-non-nullish'}>. It might not be forwarding the error properly to the Suspense boundary if errorElement is missing (or it has null or empty string).

@michenly
Copy link
Contributor Author

michenly commented Jun 4, 2024

The example had been updated without the use of useLoaderData

@brophdawg11 brophdawg11 mentioned this issue Jun 5, 2024
Merged
@brophdawg11 brophdawg11 linked a pull request Jun 5, 2024 that will close this issue
Avoid hydration loops when layout error renders also throw #9566
Merged
@brophdawg11 brophdawg11 self-assigned this Jun 5, 2024
@brophdawg11
Copy link
Contributor

The underlying issue here ends up being that Layout was throwing initially due to the invalid promise value being returned - but then when Layout was rendered again using the ErrorBoundary it would throw again. This would result in us trying to render the built-in default Remix error boundary as children of #document since we skip the <html> shell due to the presence of a root Layout. The DOM rejects this and puts React into the infinite loop.

I think the fix in the PR is safe to detect this scenario when the Layout/ErrorBoundary combo re-threw but the guidance should be that Layout/ErrorBoundary should never throw or else it defeats the purpose of using them since they'll be ignored. If useRouteLoaderData is used in Layout during an error render to bypass the can't useLoaderData in an ErrorBoundary limitation, then it should be very defensive to avoid bubbling further.

@michenly
Copy link
Contributor Author

michenly commented Jun 5, 2024

Got it! I can definitely make sure Layout/ErrorBoundary should never throw
A great caveat to note and makes a lot of sense once its explain.

Thank you so much for taking the time @brophdawg11 !

Might be worth it to mentioned in this doc along side of A note on useLoaderDatain the Layout Component ?

@michenly michenly closed this as completed Jun 5, 2024
@brophdawg11
Copy link
Contributor

Good call! Added in #9581

@michenly
Copy link
Contributor Author

michenly commented Jun 7, 2024 •
edited
Loading

Note that #9566 will fix the infinite loop and falling back to the minimal root layout when Layout throw. Re-open till the issue is close officially.

@michenly michenly reopened this Jun 7, 2024
@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Jun 14, 2024
@brophdawg11 brophdawg11 removed their assignment Jun 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.10.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions GitHub Actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.10.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Jul 3, 2024
@michenly michenly mentioned this issue Jul 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug:unverified renderer:react
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid hydration loops when layout error renders also throw remix-run/remix
4 participants
@brophdawg11 @michenly @frandiox @brookslybrand