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

refactor: error handling #1122

Merged
merged 4 commits into from
Jan 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions e2e/create-pages.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
});
Expand Down
121 changes: 15 additions & 106 deletions packages/waku/src/minimal/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
'use client';

import {
Component,
createContext,
createElement,
memo,
Expand Down Expand Up @@ -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;
Expand All @@ -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;
};
Expand Down Expand Up @@ -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],
);
};

/**
Expand All @@ -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);
Expand Down
14 changes: 2 additions & 12 deletions packages/waku/src/router/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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(
Expand Down
15 changes: 1 addition & 14 deletions packages/waku/src/router/create-pages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, '-');
Expand Down Expand Up @@ -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 () => {
Expand Down
7 changes: 1 addition & 6 deletions packages/waku/src/router/define-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ export function unstable_defineRouter(fns: {
) => Promise<{
routeElement: ReactNode;
elements: Record<SlotId, ReactNode>;
fallbackElement?: ReactNode;
}>;
getApiConfig?: () => Promise<
Iterable<{
Expand Down Expand Up @@ -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 },
);
Expand All @@ -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];
Expand Down
Loading