remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.97k stars 2.53k forks source link

An error in `MetaFunction` or `LinkFunction` is not caught by `ErrorBoundary` #3140

Closed sergiodxa closed 1 year ago

sergiodxa commented 2 years ago

What version of Remix are you using?

1.4.1

Steps to Reproduce

Create a MetaFunction with an error inside:

export let meta: MetaFunction = () => {
  throw new Error("boom")
}

Or create a LinkFunction:

export let links: LinkFunction = () => {
  throw new Error("boom")
}

This can happen if, for example, you access a property of data that doesn't exists.

Expected Behavior

It should show the ErrorBoundary from the route or parent route.

Actual Behavior

It shows a Unexpected Server Error white screen with black text and the error message below.

Screen Shot 2022-05-09 at 11 30 19
emilbryggare commented 2 years ago

This is a problem for me as well, I just wanted to add that for me the real error happened in the LoaderFunction and the MetaFunction threw an error when trying to access that data.

  1. Error in LoaderFunction
  2. Error in MetaFunction when accessing that data, e.g. trying to access X of undefined.
  3. ErrorBoundary in Route hierarchy is not activated
  4. Resulting in the white screen with Unexpected Server Error.

Do we want errors in the MetaFunction to actually trigger the ErrorBoundary? The application can probably render ok most of the time and fall back to a parent MetaFunction.

This is what I did in my application to handle this for now.

export const meta: MetaFunction = ({ data }) => {
  try {
    return getSocialMetas(data);
  } catch (error) {
    console.error(error);
    return {};
  }
};

This will fall back to the parent MetaFunction if an error in just the MetaFunction, and if the error is actually in a LoaderFunction that ErrorBoundary is activated.

bushblade commented 2 years ago

This happened to me too. I was using the meta function to add a page title based on data from my cms. The problem was that if I don't get the data I throw an error in my loader function but the meta function still tries to access the expected data. The error shows that it came from my loader function though, yet the ErrorBoundry component never renders because of the error in the meta function. It would have been easier to track down if the terminal errors say it was coming from the meta function, but they say it came from the loader function. So yeah I would agree that it would be nice if meta function errors were also caught by the ErrorBoundry.

mcansh commented 2 years ago

okay, this has do with the ErrorBoundary also rendering Links, and or Meta (which ever throws the error) as we will still call and try to render it (if you have those components in your ErrorBoundary), so it will also also throw - the current "fix" is adding a try/catch to your meta/links export, but I'm going to take a look into how we can fix that, my current thought is either pass the error to meta and links, so you could render some safer fallbacks or perhaps re-try the current ErrorBoundary without recalling links or meta. after writing this all out, i think option 1 is the better one

sergiodxa commented 2 years ago

I think passing errors in loader to MetaFunction makes sense, then ignore MetaFunction and LinksFunction errors (maybe log them to the console)

jacknevitt commented 1 year ago

I have just encountered this, and I find it quite odd that data is the thrown error instead of the expected type. I got around this by checking data instanceof Error before using properties on data. Would be nice not to do this for every meta function.

AvidDabbler commented 1 year ago

So I'm not sure if it is related, but I have a similar issue when I throw badRequest or any Error in the loader when I do not have a default exported function so the ErrorBoundary does not catch. The workaround that I found was to have an empty page return.

The reason that we want this is that we just use the index route to redirect to another page based on the user's authorized workspaces. If there are no workspaces with that user it should trigger the ErrorBoundary. This is my current workaround.

import type { LoaderArgs } from '@remix-run/node';
import { redirect } from '@remix-run/node';
import { isRouteErrorResponse, useRouteError } from '@remix-run/react';
import { type Workspace } from 'core';
import { badRequest } from 'remix-utils';
import { route } from 'routes-gen';

export async function loader({ context, request }: LoaderArgs) {
  const uow = await context.services.auth.requireAuth(request);
  const url = new URL(request.url);

  const workspaces: Workspace[] = await uow.workspaces.get();

  // Check if there are any workspaces associated with the user

  if (workspaces.length === 0) {
    throw badRequest('You are not a part of any workspaces!');
  }

  if (workspaces.some((workspace) => workspace.slug === url.hostname)) {
    return redirect(route('/:workspaceSlug', { workspaceSlug: url.hostname }));
  } else {
    return redirect(route('/:workspaceSlug', { workspaceSlug: workspaces[0].slug }));
  }
}

// the empty function that is not reachable
export default function Anything() {
  return <div></div>;
}

export function ErrorBoundary() {
  const error = useRouteError();

  if (isRouteErrorResponse(error)) {
    return (
      <div>
        <h1>Oops</h1>
        <p>Status: {error.status}</p>
        <p>{error.data.message}</p>
      </div>
    );
  }

  let errorMessage = 'Unknown error';

  return (
    <div>
      <h1>Uh oh ...</h1>
      <p>Something went wrong.</p>
      <pre>{errorMessage}</pre>
    </div>
  );
}
sergiodxa commented 1 year ago

@AvidDabbler this is expected, a route without a default export is a resource route and resource routes don't participate in the UI so they don't render an error boundary.

If you consume the resource route from a useFetcher it will handle errors, but if you go directly on the browser it will show what the loader returned, in your case the JSON of the bad request.

AvidDabbler commented 1 year ago

Awesome! thanks for the clarification

On Wed, Jun 28, 2023 at 6:13 PM Sergio Xalambrí @.***> wrote:

@AvidDabbler https://github.com/AvidDabbler this is expected, a route without a default export is a resource route and resource routes don't participate in the UI so they don't render an error boundary.

If you consume the resource route from a useFetcher it will handle errors, but if you go directly on the browser it will show what the loader returned, in your case the JSON of the bad request.

— Reply to this email directly, view it on GitHub https://github.com/remix-run/remix/issues/3140#issuecomment-1612226639, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACAUJTFRAR535G3DPIBJ643XNS277ANCNFSM5VO2Q6YA . You are receiving this because you were mentioned.Message ID: @.***>

brophdawg11 commented 1 year ago

I think the proper fix here would be to decouple the execution of meta() from the rendering of <Meta>. Right now, they're coupled in the render pass, and since <Meta> is rendered by root - there's no way to associate errors from some descendant route to the descendant error boundary. This is partially tackled here.

If we run meta after loaders during the loading phase then we could detect a failure and bubble accordingly. Probably a decently sized change though so not a bug fix for v2, more likely a larger feature we can look into once v2 goes out.

ryanflorence commented 1 year ago

I think in v2 we should at least render the root error boundary.

This all changes quite a bit with RSC (renderToPipeableStream automatically hoists meta/link/"head stuff" to the head of the document as long as those elements weren't behind a suspense boundary) which changes things considerably.

ryanflorence commented 1 year ago

Just verified that the boundary in app/root.tsx will render for these errors on latest, instead of the generic error message.

  fixture = await createFixture({
    files: {
      "app/root.tsx": js`
        import { Links, Meta, Outlet, Scripts } from "@remix-run/react";

        export default function Root() {
          return (
            <html lang="en">
              <head>
                <Meta />
                <Links />
              </head>
              <body>
                <main>
                  <Outlet />
                </main>
                <Scripts />
              </body>
            </html>
          );
        }

        export function ErrorBoundary() {
          return (
            <html>
              <head />
              <body>
                <h1>Root boundary</h1>
                <Scripts />
              </body>
            </html>
          )
        }
      `,

      "app/routes/_index.tsx": js`
        import { Link, useRouteError } from '@remix-run/react'

        export const meta = () => {
          throw new Error("lol oops")
        }

        export default function Index() {
          return <h1>Home</h1>
        }

        export function ErrorBoundary() {
          return <h1>Home boundary</h1>
        }
      `,
    },
  });
image

Ideally it would render the closest error boundary, however, we know we're going in a fairly different direction for these use cases in the future, not sure the size of the refactor here is worth the effort. Apps still control the root boundary so it's Good Enough™.