feat: Remove ErrorBoundary from Router

[rmarscher]

Would this potentially be ok? re: #967

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	Jan 10, 2025 5:30am

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[dai-shi]

This is slightly different from my intention.

Do you have an actual problem with having your own ErrorBoundary in _layout.tsx? If that's the case, I would completely remove ErrorBoundary from Router.

[rmarscher]

Do you have an actual problem with having your own ErrorBoundary in _layout.tsx? If that's the case, I would completely remove ErrorBoundary from Router.

No, I have an error boundary in my root layout. I keep hitting errors that are outside that error boundary though which are getting caught by the router error boundary. I want to control the fallback output and logging. The renderError html output is too minimal.

[rmarscher]

You could remove ErrorBoundary from Router completely and I could include one in main.tsx though. I think that would work. 👍

[dai-shi]

Let's remove ErrorBoundary from Router completely. (Prefer composition than customization.)
Should we add the default one in rscManagedPlugin?
What would happen if we have no error boundaries?

[rmarscher]

Let's remove ErrorBoundary from Router completely. (Prefer composition than customization.)

Great.

Should we add the default one in rscManagedPlugin?

Probably...

What would happen if we have no error boundaries?

I'm not sure! I'll have to try and see if it gets handled by Hono. That will help determine if we should add a default back in rscManagedPlugin.

[dai-shi]

I'm not sure! I'll have to try and see if it gets handled by Hono. That will help determine if we should add a default back in rscManagedPlugin.

Yes, please. It's not only with Hono, because the error boundary is used on the client too.

My preference is not add the default one in rscManagedPlugin.

[rmarscher]

What would happen if we have no error boundaries?

It seems to render no html output and console log the error. During dev, it also shows this warning:

An error occurred in the <InnerSlot> component.

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://react.dev/link/error-boundaries to learn more about error boundaries.

With npm start, it also clears out all the html on the page on error - inspecting the page reveals only script and modulepreload link tags. It still logs the error but doesn't have the dev warning.

For example, run the 01_template example, go to chrome devtools network tab, change throttling to offline, click on the about page link. You should hit the error and see something like this in the console:

Uncaught (in promise) TypeError: Failed to fetch dynamically imported module: http://localhost:8080/assets/rsc3-98568e70c.js

[dai-shi]

before change

after change

---

Hmmm, I'm not very happy with the default error boundary, but the blank page doesn't seem very nice either.

[dai-shi]

Although, an empty Vite / React app is the same.

[dai-shi]

I might change this decision later, but for now let's

• export ErrorBoundary from waku/router/client,
• use it in rscManagedPlugin,
• as well as <Suspense> just within <ErrorBoundary>.

Feel free to improve renderError for a good default.

[rmarscher]

Great. That sounds good. Thanks.

[rmarscher]

I made the change to export ErrorBoundary. I need to test and consider how the default might be improved.

[rmarscher]

There's an interesting test failure now in the hot reload e2e spec.

If you load a page in the dev server, navigate to a different page and then edit the source for that page to trigger a reload, there is an error caught by the default error boundary: No such element: route:/

It doesn't happen when hot reloading the same page that was initially loaded. So if I load http://localhost:3000/about directly and edit the title, it reloads without issue.

[rmarscher]

I also notice that the new default main.tsx has a hydration error:

  <ErrorBoundary>
+   <Suspense>
-   <html>

It looks like there is a default html element getting inserted somewhere that doesn't match - maybe in vite-plugin-rsc-index?

[dai-shi]

• as well as <Suspense> just within <ErrorBoundary>.

I think I was confused something. My intention was having Suspense on the server to catch server errors, but this is client side. So, let's just remove <Suspense>.

Also, I'm thinking about adding the default ErrorBoundary in DefaultRoot of create-pages.ts, instead of rscManagedPlugin. What do you think? @rmarscher @tylersayshi

[tylersayshi]

Also, I'm thinking about adding the default ErrorBoundary in DefaultRoot of create-pages.ts, instead of rscManagedPlugin.

this sounds good to me. Seems like a more proper place compared to the root layout

[dai-shi]

adding the default ErrorBoundary in DefaultRoot

Trying it and it reveals a bug...

[dai-shi]

The bug is fixed, but unreachable spec is failing, which we added in #1112.
We could change the spec, but as this can happen very easily (and people see the blank page), I wonder if the decision to put the default error boundary in DefaultRoot is right.

Is it normal too see the blank page with the client navigation error? Vite React does it though. How about other React frameworks? Next? Remix?

[rmarscher]

How about other React frameworks? Next? Remix?

They both implement custom ErrorBoundaries to allow components to throw routing errors during render - 401, 404, etc - that get caught and translated into server response codes. Remix has a useRouteError hook that makes the error available.

I think they also use it for redirects - so redirects throw a special error object that instructs the server to return a 307 or 308 error code and Location header.

That's something we could implement in userland, but maybe worth putting into Waku.

NextJS has special file-system naming for defining global and route error boundaries.

Remix lets you define an ErrorBoundary for each route component. I can see here that they load a RemixRootDefaultErrorBoundary. https://github.com/remix-run/remix/blob/v2.11.1/packages/remix-react/routes.tsx#L74-L126

https://github.com/remix-run/remix/blob/main/packages/remix-react/errorBoundaries.tsx

Remix's default is pretty basic like ours has been. It's an H1 tag with the error type and error. Plus it displays the stack trace if in dev mode. It also uses console.log to try to send a message to the developer.
"💿 Hey developer 👋. You can provide a way better UX than this when your app throws errors. Check out https://remix.run/guides/errors for more information."
https://github.com/remix-run/remix/blob/542310d95cba4ac41eb70cbd26a7c40a9e31090e/packages/remix-react/errorBoundaries.tsx#L18

Remix has an interesting hydration fallback component and error boundary too.

ref:
https://nextjs.org/docs/app/building-your-application/routing/error-handling#using-error-boundaries
https://remix.run/docs/en/main/route/error-boundary