refactor: error handling (#1122)

This is to follow up #1112.
dai-shi · Jan 5, 2025 · 3140926 · 3140926
1 parent 0a6f55b
commit 3140926
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 140 deletions.
diff --git a/e2e/create-pages.spec.ts b/e2e/create-pages.spec.ts
@@ -112,8 +112,7 @@ for (const mode of ['DEV', 'PRD'] as const) {
       await page.click("a[href='/error']");
       // Default router client error boundary is reached
       await expect(
-        // TODO "Not Found" isn't appropriate for "unreachable error"
-        page.getByRole('heading', { name: 'Not Found' }),
+        page.getByRole('heading', { name: 'Failed to Fetch' }),
       ).toBeVisible();
       ({ port, stopApp } = await startApp(mode));
     });

diff --git a/packages/waku/src/minimal/client.ts b/packages/waku/src/minimal/client.ts
@@ -2,7 +2,6 @@
 'use client';
 
 import {
-  Component,
   createContext,
   createElement,
   memo,
@@ -49,9 +48,7 @@ const checkStatus = async (
   return response;
 };
 
-type Elements = Promise<Record<string, ReactNode>> & {
-  prev?: Record<string, ReactNode> | undefined;
-};
+type Elements = Promise<Record<string, ReactNode>>;
 
 const getCached = <T>(c: () => T, m: WeakMap<object, T>, k: object): T =>
   (m.has(k) ? m : m.set(k, c())).get(k) as T;
@@ -63,21 +60,9 @@ const mergeElements = (a: Elements, b: Elements): Elements => {
         .then(([a, b]) => {
           const nextElements = { ...a, ...b };
           delete nextElements._value;
-          promise.prev = a;
           resolve(nextElements);
         })
-        .catch((e) => {
-          a.then(
-            (a) => {
-              promise.prev = a;
-              reject(e);
-            },
-            () => {
-              promise.prev = a.prev;
-              reject(e);
-            },
-          );
-        });
+        .catch((e) => reject(e));
     });
     return promise;
   };
@@ -255,60 +240,24 @@ export const useRefetch = () => use(RefetchContext);
 const ChildrenContext = createContext<ReactNode>(undefined);
 const ChildrenContextProvider = memo(ChildrenContext.Provider);
 
-type OuterSlotProps = {
-  elementsPromise: Elements;
-  unstable_shouldRenderPrev:
-    | ((err: unknown, prevElements: Record<string, ReactNode>) => boolean)
-    | undefined;
-  renderSlot: (elements: Record<string, ReactNode>, err?: unknown) => ReactNode;
-  children?: ReactNode;
-};
-
-class OuterSlot extends Component<OuterSlotProps, { error?: unknown }> {
-  constructor(props: OuterSlotProps) {
-    super(props);
-    this.state = {};
-  }
-  static getDerivedStateFromError(error: unknown) {
-    return { error };
-  }
-  render() {
-    if ('error' in this.state) {
-      const e = this.state.error;
-      if (e instanceof Error && !('statusCode' in e)) {
-        // HACK we assume any error as Not Found,
-        // probably caused by history api fallback
-        (e as any).statusCode = 404;
-      }
-      const prevElements = this.props.elementsPromise.prev;
-      if (
-        prevElements &&
-        this.props.unstable_shouldRenderPrev?.(e, prevElements)
-      ) {
-        return this.props.renderSlot(prevElements, e);
-      } else {
-        throw e;
-      }
-    }
-    return this.props.children;
-  }
-}
-
 const InnerSlot = ({
+  id,
   elementsPromise,
-  renderSlot,
+  children,
 }: {
+  id: string;
   elementsPromise: Elements;
-  renderSlot: (elements: Record<string, ReactNode>, err?: unknown) => ReactNode;
+  children?: ReactNode;
 }) => {
   const elements = use(elementsPromise);
-  return renderSlot(elements);
-};
-
-const ErrorContext = createContext<unknown>(undefined);
-export const ThrowError_UNSTABLE = () => {
-  const err = use(ErrorContext);
-  throw err;
+  if (!(id in elements)) {
+    throw new Error('No such element: ' + id);
+  }
+  return createElement(
+    ChildrenContextProvider,
+    { value: children },
+    elements[id],
+  );
 };
 
 /**
@@ -328,55 +277,15 @@ export const ThrowError_UNSTABLE = () => {
 export const Slot = ({
   id,
   children,
-  fallback,
-  unstable_shouldRenderPrev,
-  unstable_renderPrev,
 }: {
   id: string;
   children?: ReactNode;
-  fallback?: ReactNode;
-  unstable_shouldRenderPrev?: (
-    err: unknown,
-    prevElements: Record<string, ReactNode>,
-  ) => boolean;
-  unstable_renderPrev?: boolean;
 }) => {
   const elementsPromise = use(ElementsContext);
   if (!elementsPromise) {
     throw new Error('Missing Root component');
   }
-  const renderSlot = (elements: Record<string, ReactNode>, err?: unknown) => {
-    if (!(id in elements)) {
-      if (fallback) {
-        if (err) {
-          // HACK I'm not sure if this is the right way
-          return createElement(ErrorContext.Provider, { value: err }, fallback);
-        }
-        return fallback;
-      }
-      throw new Error('Not found: ' + id);
-    }
-    return createElement(
-      ChildrenContextProvider,
-      { value: children },
-      elements[id],
-    );
-  };
-  if (unstable_renderPrev) {
-    if (!elementsPromise.prev) {
-      throw new Error('Missing prev elements');
-    }
-    return renderSlot(elementsPromise.prev);
-  }
-  return createElement(
-    OuterSlot,
-    {
-      elementsPromise,
-      unstable_shouldRenderPrev,
-      renderSlot,
-    },
-    createElement(InnerSlot, { elementsPromise, renderSlot }),
-  );
+  return createElement(InnerSlot, { id, elementsPromise }, children);
 };
 
 export const Children = () => use(ChildrenContext);

diff --git a/packages/waku/src/router/client.ts b/packages/waku/src/router/client.ts
@@ -289,11 +289,8 @@ class ErrorBoundary extends Component<
   }
   render() {
     if ('error' in this.state) {
-      if (
-        this.state.error instanceof Error &&
-        (this.state.error as any).statusCode === 404
-      ) {
-        return renderError('Not Found');
+      if (this.state.error instanceof Error) {
+        return renderError(this.state.error.message);
       }
       return renderError(String(this.state.error));
     }
@@ -413,13 +410,6 @@ const InnerRouter = ({
 
   const routeElement = createElement(Slot, {
     id: getRouteSlotId(route.path),
-    unstable_shouldRenderPrev: (_err, prevElements) =>
-      // HACK this might not work in some cases
-      'fallback' in prevElements,
-    fallback: createElement(Slot, {
-      id: 'fallback',
-      unstable_renderPrev: true,
-    }),
   });
 
   return createElement(

diff --git a/packages/waku/src/router/create-pages.ts b/packages/waku/src/router/create-pages.ts
@@ -15,7 +15,7 @@ import type {
   GetSlugs,
   PropsForPages,
 } from './create-pages-utils/inferred-path-types.js';
-import { Children, Slot, ThrowError_UNSTABLE } from '../minimal/client.js';
+import { Children, Slot } from '../minimal/client.js';
 
 const sanitizeSlug = (slug: string) =>
   slug.replace(/\./g, '').replace(/ /g, '-');
@@ -591,19 +591,6 @@ export const createPages = <
           { id: 'root' },
           createNestedElements(routeChildren),
         ),
-        // HACK this is hard-coded convention
-        // FIXME we should revisit the error boundary use case design
-        fallbackElement: createElement(
-          Slot,
-          { id: 'root', unstable_renderPrev: true },
-          layoutPaths.includes('/')
-            ? createElement(
-                Slot,
-                { id: 'layout:/', unstable_renderPrev: true },
-                createElement(ThrowError_UNSTABLE),
-              )
-            : createElement(ThrowError_UNSTABLE),
-        ),
       };
     },
     getApiConfig: async () => {

diff --git a/packages/waku/src/router/define-router.ts b/packages/waku/src/router/define-router.ts
@@ -90,7 +90,6 @@ export function unstable_defineRouter(fns: {
   ) => Promise<{
     routeElement: ReactNode;
     elements: Record<SlotId, ReactNode>;
-    fallbackElement?: ReactNode;
   }>;
   getApiConfig?: () => Promise<
     Iterable<{
@@ -210,7 +209,7 @@ export function unstable_defineRouter(fns: {
     }
     const skip = isStringArray(skipParam) ? skipParam : [];
     const { query } = parseRscParams(rscParams);
-    const { routeElement, elements, fallbackElement } = await fns.handleRoute(
+    const { routeElement, elements } = await fns.handleRoute(
       pathname,
       pathConfigItem.specs.isStatic ? {} : { query },
     );
@@ -222,10 +221,6 @@ export function unstable_defineRouter(fns: {
     const entries = {
       ...elements,
       [ROUTE_SLOT_ID_PREFIX + pathname]: routeElement,
-      ...((fallbackElement ? { fallback: fallbackElement } : {}) as Record<
-        string,
-        ReactNode
-      >),
     };
     for (const skipId of await filterEffectiveSkip(pathname, skip)) {
       delete entries[skipId];