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

Single Fetch throwing response does not work #9793

Closed
timvandam opened this issue Jul 24, 2024 · 9 comments
Closed

Single Fetch throwing response does not work #9793

timvandam opened this issue Jul 24, 2024 · 9 comments

Comments

@timvandam
Copy link

Reproduction

https://stackblitz.com/edit/remix-run-remix-ndbnrb?file=app%2Froutes%2F_index.tsx,app%2Froot.tsx,tsconfig.json

System Info

System:
    OS: macOS 14.3
    CPU: (16) arm64 Apple M3 Max
    Memory: 229.22 MB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.1/bin/npm
    pnpm: 9.5.0 - ~/.nvm/versions/node/v20.11.1/bin/pnpm
  Browsers:
    Chrome: 127.0.6533.72
    Safari: 17.3

Used Package Manager

npm

Expected Behavior

Loader states should always stay consistent. E.g. useLoaderData<typeof loader> should always contain the latest valid loader data

Actual Behavior

Loader state isnt consistent when using throw response with single fetch. It becomes undefined unexpectedly leading to destructuring errors (see stackblitz)

@brophdawg11
Copy link
Contributor

If you scroll, up in the console you will see this error after submitting:

You cannot useLoaderData in an errorElement (routeId: root)

This is standard behavior in all Remix routes. When an ErrorBoundary is rendering, you cannot use useLoaderData because it may have been the loader that threw the error so it could be undefined and then the types would be incorrect.

In your case, you have a useLoaderData in the root Layout which works fine on initial load. Then when you submit from the child route and it throws, that error bubbles up to the default ErrorBoundary which renders inside the Layout - and that call of useLoaderData fails and logs the error.

This is a common enough occurrence that there's a section in the docs, see the "A note on useLoaderData in the Layout Component" section in the root route docs.

If you need to display root loader data in Layout, then it's best to use useRouteLoaderData("root") which will include undefined in the types and force you to be defensive in case it was the root loader that threw the error.

@brophdawg11 brophdawg11 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2024
@timvandam
Copy link
Author

@brophdawg11 Thanks for your explanation, however, I am throwing the response stub which per the docs is a way to the short circuit actions/loaders to immediately respond to the incoming request. So what is happening here is that an error is incorrectly thrown by Remix, while it should have just sent a normal response

@brophdawg11
Copy link
Contributor

So what is happening here is that an error is incorrectly thrown by Remix

How so? Your code is throwing the response stub - and any time you throw anything from a loader/action you trigger an ErrorBoundary.

Can you point to the section of the docs that is causing the confusion?

@timvandam
Copy link
Author

So what is happening here is that an error is incorrectly thrown by Remix

How so? Your code is throwing the response stub - and any time you throw anything from a loader/action you trigger an ErrorBoundary.

Can you point to the section of the docs that is causing the confusion?

That is not the behavior in non-single-fetch mode when throwing Response instances, right? E.g., throw redirect(). For Single Fetch, the docs state:

You can also throw these response stubs to short circuit the flow of your loaders and actions:

export const loader = defineLoader(
  ({ request, response }) => {
    if (shouldRedirectToHome(request)) {
      response.status = 302;
      response.headers.set("Location", "/");
      throw response;
    }
    // ...
  }
);

(https://remix.run/docs/en/main/guides/single-fetch)

@brophdawg11
Copy link
Contributor

Redirects are different than errors since you're telling the router to navigate to a new route, as opposed to informing it of an error on the current route.

You're not throwing a redirect in your reproduction though - you're throwing a response stub with a 400 status. Without single fetch, that's equivalent to throw json({ error: "broken" }, { status: 400 }) which would trigger the ErrorBoundary in the same way.

@timvandam
Copy link
Author

timvandam commented Jul 25, 2024

Redirects are different than errors since you're telling the router to navigate to a new route, as opposed to informing it of an error on the current route.

You're not throwing a redirect in your reproduction though - you're throwing a response stub with a 400 status. Without single fetch, that's equivalent to throw json({ error: "broken" }, { status: 400 }) which would trigger the ErrorBoundary in the same way.

You're right, I've only ever thrown with redirect before so didn't realize it worked this way. I notice though that single fetch's types override useRouteLoaderData always return something, i.e., it removes the | undefined from the return type (see

export function useRouteLoaderData<T extends Loader>(
routeId: string
): Serialize<T>;
). This seems inconsistent with this behavior

Edit: after testing this out it seems like useRouteLoaderData is now always set, so while useLoaderData becomes undefined on error useRouteLoaderData retains its value from before the error

@brophdawg11
Copy link
Contributor

it removes the | undefined from the return type

Ah thanks for pointing this out - this is a bug. In your case useRouteLoaderData("root") will always be defined but if you put an invalid route id or a non-mounted id in there it would be undefined - got this fixed in #9796 👍

@timvandam
Copy link
Author

How exactly is the behavior different between the two, and for what reason? It’s not clear from the docs. I only seitched to it for the | ubdefined and was surprised it never became undefined during errors unlike useLoaderData

@brophdawg11
Copy link
Contributor

useLoaderData is the happy path. Because you're using it in the specific route and that route will only ever be rendered after the loader has completed successfully - it should always return data. That's why we don't allow it in error boundaries - because we cant guarantee the data will be there. And thus it's typed accordingly, without | undefined.

useRouteLoaderData lets you ask for any route's data - and since we have no idea if the route you're asking for is mounted, it can be undefined - and you can then use that as the workaround for the error boundary because the typings are accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants