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

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

Closed
sergiodxa opened this issue May 9, 2022 · 11 comments
Closed
Assignees
Labels
bug Something isn't working feat:links-meta Issues related to links()/meta()

Comments

@sergiodxa
Copy link
Member

sergiodxa commented May 9, 2022

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

@sergiodxa sergiodxa changed the title An error in MetaFunction is not caught by ErrorBoundary An error in MetaFunction or LinkFunction is not caught by ErrorBoundary May 9, 2022
@emilbryggare
Copy link
Contributor

emilbryggare commented May 26, 2022

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.

@MichaelDeBoey MichaelDeBoey added bug Something isn't working and removed bug:unverified labels May 26, 2022
@MichaelDeBoey MichaelDeBoey changed the title An error in MetaFunction or LinkFunction is not caught by ErrorBoundary An error in MetaFunction or LinkFunction is not caught by ErrorBoundary May 26, 2022
@bushblade
Copy link

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
Copy link
Collaborator

mcansh commented Jul 11, 2022

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
Copy link
Member Author

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

@jacknevitt
Copy link

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
Copy link

AvidDabbler commented Jun 28, 2023

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
Copy link
Member Author

@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
Copy link

AvidDabbler commented Jun 28, 2023 via email

@brophdawg11
Copy link
Contributor

brophdawg11 commented Aug 2, 2023

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat:links-meta Issues related to links()/meta()
Projects
No open projects
Status: Closed
9 participants