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

Meta function can't access loader data when it throws #3903

Closed
janhesters opened this issue Aug 2, 2022 · 8 comments
Closed

Meta function can't access loader data when it throws #3903

janhesters opened this issue Aug 2, 2022 · 8 comments
Assignees
Labels
feat:links-meta Issues related to links()/meta()

Comments

@janhesters
Copy link

janhesters commented Aug 2, 2022

Context

Why is this important? Screen reader users often rely on title's to infer the content of a page. And the loader is used to dynamically set the title property in meta functions.

The inability to set title's dynamically on errors severely hinders this functionality.

What version of Remix are you using?

1.6.7

Steps to Reproduce

You need a loader that throws.

import type { MetaFunction } from '@remix-run/node';
import { json } from '@remix-run/node';
import { Outlet, useCatch } from '@remix-run/react';

export const loader = () => {
  if (Math.random() > 0.5) {
    throw json(
      { message: 'not found', slug: 'foo', title: 'yolo' },
      { status: 400 },
    );
  }

  return json({ title: 'Not throwing' });
};

export const meta: MetaFunction = parameters => {
  console.log(parameters);
  return {};
};

export default function MyComponent() {
  return <p>Hello</>;
}

export function CatchBoundary() {
  const caught = useCatch();

  return (
    <p>
      {caught.status}: {caught.data.slug}
    </p>
  );
}

Expected Behavior

The meta function should have access to the data of json that was thrown.

Actual Behavior

The meta function doesn't see the loader's data.

Possible Alternative

In case of errors you can set the title in the higher routes that didn't throw, but that would require all routes always getting all titles, which is bad for performance 😕

Or maybe there is another way to set the title in CatchBoundarys?

@kiliman
Copy link
Collaborator

kiliman commented Aug 2, 2022

If you throw a response, it's not valid data from your loader, hence loader data is not present in your meta function.

If you want to update the title from your <CatchBoundary/> you can add a useEffect

export function CatchBoundary() {
  const caught = useCatch()
  useEffect(() => {
    if (caught?.data?.title) {
      document.title = caught.data.title
    }
  })
  return (
    <>
      <h1>CatchBoundary</h1>
      <p>Title should be {caught.data.title}</p>
      <p>
        {caught.status}: {caught.data.slug}
      </p>
    </>
  )
}

https://codesandbox.io/s/remix-catchboundary-title-h2nske?file=/app/routes/index.tsx

@janhesters
Copy link
Author

@kiliman Thank you for this!

If you throw a response, it's not valid data from your loader, hence loader data is not present in your meta function.

Hmm, I don't get that. Why would we assume that? Wouldn't it make more sense to add some conditionals into the meta function so it can handle happy as well as sad paths?

If you want to update the title from your you can add a useEffect

This definitely solves the issue, but seems like a hacky solution considering the Remix convention is to do it with meta 🤔

@kiliman
Copy link
Collaborator

kiliman commented Aug 3, 2022

When you throw from a function, you don't get the same result as if the function returned data. The meta function currently receives the returned loader data.

I'm not saying this can't be changed. Just explaining how it currently works today.

I guess my only concern is that if this was changed, would it be a breaking change? Since meta doesn't expect thrown data, it may not be in the same shape as loader data, so existing meta functions could break if they don't check. I think the only way it would work is if they simply added another property to the meta arguments. Something like catchData. Then existing code will work and new code can inspect that property as needed.

@ryanflorence I read in the decision doc for remix router/react-router that there may be no distinction between Error and Catch boundaries in the future. Have you considered adding the error and catch data to the meta function? This way developers can have more control of the UI being generated in cases of non-happy paths.

@bencallaway
Copy link

@janhesters I ran across this issue as well, and settled on another admittedly hacky solution, albeit one that renders on the server-side. Here's a simplified version of my root component using a module-level closure to determine its meta title.

app/root.tsx
import type { MetaFunction } from "@remix-run/node";
import { Meta, Outlet, useCatch } from "@remix-run/react";

let metaTitle = "Initial title";

export const meta: MetaFunction = () => {
  return {
    title: metaTitle,
  };
};

function Document(props: { children: React.ReactNode }) {
  return (
    <html>
      <head>
        <Meta />
      </head>
      <body>{props.children}</body>
    </html>
  );
}

export default function Root() {
  return (
    <Document>
      <Outlet />
    </Document>
  );
}

export function CatchBoundary() {
  const caught = useCatch();

  metaTitle = `${caught.status}: ${caught.statusText}`

  return (
    <Document>
      <p>{caught.data}</p>
    </Document>
  );
}
app/routes/index.tsx
import type { LoaderFunction } from "@remix-run/node";
import { useLoaderData } from "@remix-run/react";

export const loader: LoaderFunction = ({ request }) => {
  if (request.url.match(/[?|&]oops=/)) {
    throw new Response("Oops!", { status: 400, statusText: "Bad request" });
  }
  return new Response("Success!");
};

export default function IndexRoute() {
  const content = useLoaderData<string>();
  return <p>{content}</p>;
}

Navigate to http:/localhost:3000/ and the initial title is rendered.
Navigate to http:/localhost:3000/?oops and the title assigned within the CatchBoundary is rendered.

On the face of it, passing error/catch data to a MetaFunction would come in handy

@machour
Copy link
Collaborator

machour commented Jan 22, 2023

@chaance is this handled by the new Meta API? 🙏🏼

@chaance
Copy link
Collaborator

chaance commented Jan 22, 2023

@chaance is this handled by the new Meta API? 🙏🏼

No. We're aware of this limitation. I'll make sure something is on our roadmap if it isn't already.

@brophdawg11 brophdawg11 added the feat:links-meta Issues related to links()/meta() label May 5, 2023
ryanflorence added a commit that referenced this issue Aug 8, 2023
ryanflorence added a commit that referenced this issue Aug 8, 2023
ryanflorence added a commit that referenced this issue Aug 8, 2023
ryanflorence added a commit that referenced this issue Aug 8, 2023
ryanflorence added a commit that referenced this issue Aug 8, 2023
ryanflorence added a commit that referenced this issue Aug 8, 2023
ryanflorence added a commit that referenced this issue Aug 10, 2023
MichaelDeBoey pushed a commit that referenced this issue Aug 16, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-1d3d86e-20230817 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
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat:links-meta Issues related to links()/meta()
Projects
None yet
Development

No branches or pull requests

7 participants