solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.11k stars 138 forks source link

[Bug?]: Unable to use `ErrorBoundary` when throwing Error from cached function in combination with `load` #399

Closed bondehagen closed 2 months ago

bondehagen commented 3 months ago

Duplicates

Latest version

Current behavior 😯

Node process exits with unhandled exception

Expected behavior 🤔

Load function should ignore errors and ErrorBoundary should get triggered.

Steps to reproduce 🕹

Steps:

import { Show, ErrorBoundary } from "solid-js";
import { type RouteDefinition, type RouteSectionProps, cache, createAsync } from "@solidjs/router";
import { HttpStatusCode } from "@solidjs/start";

export const route = {
  load: ({ params }) => getHouse(params.house)
} satisfies RouteDefinition;

const getHouse = cache(async (house: string) => {
  "use server"
  if (house != "gryffindor") {
    throw new Error("House not found");
  }
  return house;
}, "house");

export default function House(props: RouteSectionProps) {
  const house = createAsync(() => getHouse(props.params.house), {
    deferStream: true,
  });
  return (
    <ErrorBoundary
      fallback={(e) => (
        <Show when={e.message === "House not found"}>
          <HttpStatusCode code={404} />
          <h1>House not found</h1>
        </Show>
      )}
    >
      <div>{house()}</div>
    </ErrorBoundary>
  );
}
  1. Run http://localhost:3000/house/gryffindor and make sure that page works
  2. Run http://localhost:3000/house/notgryffindor and see that node process fail with unhandlet exception

Context 🔦

It should be possible to handle errors in combination with preloading data.

Your environment 🌎

System:
 OS: Windows

Node: v21.1.0

Packages:
 vinxi: 0.3.11
 solid-js: 1.8.16
 @solidjs/router: 0.13.1
 @solidjs/start: 1.0.0-rc.0
ryansolid commented 3 months ago

Yeah this looks like a router issue. I'm going to move it there.

ryansolid commented 3 months ago

Interesting.. The problem here is that unless the promises are awaited we can't catch them. They will be uncaught. Like I can't just wrap the load function in a try {} finally {}. It won't do anything. But it also doesn't make a ton of sense to await them here. In the browser this would be inconsequential but I'm gathering the node process is exiting.

paularmstrong commented 3 months ago

This just hit me as well. The node process is exiting on the uncaught exception. It does not require having a route load function for me:

node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

Error
    at eval (/.../src/routes/path/to/file.tsx?pick=route:20:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
indeyets commented 3 months ago

Connecting .catch(…) to them (like we did with promises before async/await was invented) should probably still work.

ryansolid commented 3 months ago

For the end user but not something we can do with the library. I mean hmm.. I suppose the cached functions could not throw under some circumstances.