Skip to content

Commit

Permalink
fix: revamp error management to make it more robust
Browse files Browse the repository at this point in the history
- option to return to previous page on errors (javascript only feature essentially)
- better handling of different cases
  - handle error messages the same way if it's a page / server / action error
  - properly handle query errors
  - allow error page to possess i18nPayload data
  - allow `error` sveltekit helper to accept i18nPayload data
- properly handle tsRestResult when not ok (error shown shouldn't be only the status number, but the actual error message)
- prevent redirect loop when redirecting from error (will use the error helper instead of the redirect if a redirect loop is detected)
  • Loading branch information
V-ed committed Oct 13, 2024
1 parent 43dd082 commit 8fb9300
Show file tree
Hide file tree
Showing 13 changed files with 149 additions and 31 deletions.
2 changes: 1 addition & 1 deletion api/src/@common/users/admin/admin.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export class AdminController {
return {
status: 400,
body: {
message: 'No user with email!',
message: `No user with email "${email}"!`,
},
};
}
Expand Down
7 changes: 6 additions & 1 deletion client/src/app.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ export interface AppPageData extends PageMessages {
flash?: PageMessages;
}

export interface AppError {
i18nPayload?: Record<string, unknown>;
}
export type AppErrorBody = string | App.Error;

// See https://kit.svelte.dev/docs/types#app
// for information about these interfaces
declare global {
Expand All @@ -33,7 +38,7 @@ declare global {
namespace App {
interface Locals extends AppLocals {}
interface PageData extends AppPageData {}
// interface Error {}
interface Error extends AppError {}
// interface Platform {}
}
}
5 changes: 5 additions & 0 deletions client/src/i18n/en/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@
"default": "An unknown error as happened..."
}
}
},
"previous-page": "Go to Previous Page",
"query": {
"single": "There was an error with your query : {{error}}",
"multiple": "There were many issues with your query : {{errors}}"
}
}
}
5 changes: 5 additions & 0 deletions client/src/i18n/fr/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@
"already-home": {
"summary": "Habituellement, il y a un bouton ici vous indiquant d'aller à la page d'accueil, mais il semble que vous soyez déjà dans la page d'accueil...",
"detail": "Cependant, ne vous inquiétez pas, ce problème est actuellement en train de se faire réparer!"
},
"previous-page": "Aller à la page précédente",
"query": {
"single": "Une erreur s'est produite avec votre requête : {{error}}",
"multiple": "Plusieurs problèmes sont survenus avec votre requête : {{errors}}"
}
},
"confirm": "Confirmer"
Expand Down
11 changes: 11 additions & 0 deletions client/src/i18n/translations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,17 @@ export async function loadI18n(locale: string, route: string) {
locale: i18nInstance.locale,
locales: i18nInstance.locales,
setLocale: i18nInstance.setLocale,
tPayload: <T extends Record<string, unknown> | undefined>(
i18nPayload: T,
): T extends undefined ? undefined : Record<keyof T, string> => {
if (!i18nPayload) {
return undefined as never;
}

const translatedEntries = Object.entries(i18nPayload).map(([key, value]) => [key, i18nInstance.t.get(String(value))]);

return Object.fromEntries(translatedEntries);
},
};
}

Expand Down
23 changes: 18 additions & 5 deletions client/src/lib/components/PageError.svelte
Original file line number Diff line number Diff line change
@@ -1,26 +1,39 @@
<script lang="ts">
import { page } from '$app/stores';
import { getI18n } from '$i18n';
import { previousPage } from '$lib/stores';
import { reconstructUrl } from '$lib/utils/urls';
import { Alert, Button } from 'flowbite-svelte';
import Icon from './Icon.svelte';
let i18n = getI18n();
$: ({ t } = $i18n);
$: ({ t, tPayload } = $i18n);
export let icon: string;
export let errorMessage: string;
export let i18nPayload: Record<string, unknown> | undefined = undefined;
$: previousPageUrl = $previousPage && $previousPage !== reconstructUrl($page.url) ? $previousPage : undefined;
</script>

<div class="flex flex-col self-center items-center gap-10">
<div class="flex items-center">
<div class="flex gap-3 items-center flex-col sm:flex-row">
<Icon class="{icon} s-48"></Icon>
<span class="text-lg text-center">{$t(errorMessage)}</span>
<span class="text-lg text-center">{$t(errorMessage, tPayload(i18nPayload))}</span>
</div>

<slot />

<slot name="redirect">
{#if $page.url.pathname !== '/'}
<Button href="/">{$t('common.errorpage.home-button')}</Button>
{#if previousPageUrl || $page.url.pathname !== '/'}
<div class="flex gap-3 flex-col sm:flex-row">
{#if previousPageUrl}
<Button href={previousPageUrl}>{$t('common.errorpage.previous-page')}</Button>
{/if}

{#if $page.url.pathname !== '/'}
<Button href="/">{$t('common.errorpage.home-button')}</Button>
{/if}
</div>
{:else}
<Alert color="yellow" class="flex flex-col gap-3 items-center">
<span>{$t('common.errorpage.already-home.summary')}</span>
Expand Down
22 changes: 21 additions & 1 deletion client/src/lib/stores/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,32 @@
import { navigating } from '$app/stores';
import type { SessionUser } from '$auth/auth-handler';
import { writable } from 'svelte/store';
import { reconstructUrl } from '$lib/utils/urls';
import { readable, writable } from 'svelte/store';
import { createStoreContext } from './utils/context';
import { useToggleable } from './utils/toggleable';

export const isDrawerHidden = useToggleable(writable(true), (drawerOpenStore) => drawerOpenStore.update((isOpen) => !isOpen));

export const { getStore: getSessionUser, setStore: setSessionUser } = createStoreContext<SessionUser>();

export const previousPage = readable<string | undefined>(undefined, (set) => {
const unsubscribe = navigating.subscribe(($navigating) => {
// Check if `$navigating` has a value
// because it's set to `null` after navigation is done
if ($navigating) {
const fromUrl = $navigating.from?.url;

if (!fromUrl) {
set(undefined);
} else {
set(reconstructUrl(fromUrl));
}
}
});

return () => unsubscribe();
});

export * from './theme';
export * from './utils/matchMedia';
export * from './utils/storage/local-storage';
7 changes: 4 additions & 3 deletions client/src/lib/ts-rest/client.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { AppErrorBody } from '$app-types';
import type { PageMessages } from '$lib/types';
import { getApiUrl } from '$lib/utils';
import { r, tsRestFetcherApi, type ApiFetcherData, type CommonError } from '@app/contract';
Expand All @@ -9,7 +10,7 @@ export type ErrorPageMessagesData = PageMessages | ((args: OnErrorFunctionArgs)

export type OnErrorFunctionArgs<T = unknown> = {
/** The error message. Usually a stringified `data.body.message`, but could be other things depending on the error. */
message: string;
message: AppErrorBody;
data: ApiFetcherData<T>;
event?: RequestEvent;
};
Expand Down Expand Up @@ -52,9 +53,9 @@ export type ApiResponseHandlerOptions = {
skipErrorHandling?: boolean;
};

const apiUrl = getApiUrl();

export function createTsRestClient(event?: RequestEvent) {
const apiUrl = getApiUrl();

const client = initClient(r, {
baseUrl: apiUrl.href.slice(0, -1),
baseHeaders: {},
Expand Down
62 changes: 48 additions & 14 deletions client/src/lib/ts-rest/errorHandler.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import type { AppErrorBody } from '$app-types';
import { createToasts } from '$lib/components/ToastManager/helper';
import type { PageMessages } from '$lib/types';
import { flashStore } from '$lib/utils/flash';
import { extractFlashMessageFromEvent, flashStore } from '$lib/utils/flash';
import { commonErrors, isCommonError, type ApiFetcherData, type CommonError } from '@app/contract';
import { error, type RequestEvent } from '@sveltejs/kit';
import { StatusCodes } from 'http-status-codes';
import { redirect } from 'sveltekit-flash-message/server';
import type { ZodError } from 'zod';
import type { ApiResponseHandlerOptions, ErrorPageMessagesData, OnErrorFunctionArgs } from './client';

function getErrorPageMessagesData(handler: ErrorPageMessagesData | undefined, args: OnErrorFunctionArgs) {
Expand All @@ -19,12 +21,34 @@ function getErrorPageMessagesData(handler: ErrorPageMessagesData | undefined, ar
return handler;
}

export async function extractErrorMessageFromApiFetcherData(data: ApiFetcherData) {
export function extractErrorMessageFromApiFetcherData(data: ApiFetcherData): AppErrorBody | Promise<AppErrorBody> {
const body = data.body as Record<string, unknown> | Blob;

const message = body instanceof Blob ? await body.text() : 'message' in body ? String(body.message) : JSON.stringify(data.body);
if (body instanceof Blob) {
return body.text();
}

if ('message' in body) {
return String(body.message);
}

if ('queryResult' in body) {
const { issues } = body.queryResult as { issues: ZodError['issues'] };

if (issues.length === 1) {
return {
message: 'common.errorpage.query.single' satisfies I18nKey,
i18nPayload: { error: issues[0]!.message },
};
}

return {
message: 'common.errorpage.query.multiple' satisfies I18nKey,
i18nPayload: { errors: issues.map((issue) => issue.message).join(', ') },
};
}

return message;
return JSON.stringify(data.body);
}

export async function checkForApiErrors(data: ApiFetcherData, options?: ApiResponseHandlerOptions) {
Expand All @@ -49,7 +73,7 @@ export async function checkForApiErrors(data: ApiFetcherData, options?: ApiRespo
isErrorHandled = !!(await onBadRequest?.({ event, data, message }));

if (!isErrorHandled) {
routeError(data.status, data.body.message, {
routeError(data.status, message, {
event,
doThrowForPage: showErrorPageOnBadRequest,
pageMessagesData: await getErrorPageMessagesData(errorPageMessagesData, { event, data, message }),
Expand Down Expand Up @@ -103,12 +127,13 @@ export type RouteErrorOptions = Partial<{
pageMessagesData?: PageMessages;
}>;

export function routeError(status: number, message: string, options?: RouteErrorOptions) {
export function routeError(status: number, message: string | App.Error, options?: RouteErrorOptions) {
const { event, doThrowForPage = true, pageMessagesData } = options ?? {};

const errorToasts = createToasts({
text: message,
text: typeof message === 'string' ? message : message.message,
type: 'error',
i18nPayload: typeof message === 'object' ? message.i18nPayload : undefined,
});

if (event) {
Expand All @@ -124,13 +149,22 @@ export function routeError(status: number, message: string, options?: RouteError
return;
}

redirect(
{
toasts: errorToasts,
...pageMessagesData,
},
event,
);
const flash = extractFlashMessageFromEvent(event);

const isFromRedirect = 'isFromRedirect';

if (flash?.[isFromRedirect]) {
error(status as never, message);
} else {
redirect(
{
toasts: errorToasts,
...pageMessagesData,
[isFromRedirect]: true,
},
event,
);
}
}

if (doThrowForPage) {
Expand Down
20 changes: 17 additions & 3 deletions client/src/lib/utils/assertions.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,35 @@
import { createLayoutAlert, type LayoutAlertData } from '$lib/components/LayoutAlert/helper';
import { createToasts, type ToastData, type ToastManagerData } from '$lib/components/ToastManager/helper';
import { extractErrorMessageFromApiFetcherData } from '$lib/ts-rest/errorHandler';
import type { PageMessages } from '$lib/types';
import { error, fail, type RequestEvent } from '@sveltejs/kit';
import { StatusCodes } from 'http-status-codes';
import { redirect } from 'sveltekit-flash-message/server';
import type { Infer, SuperValidated } from 'sveltekit-superforms';
import type { AnyZodObject } from 'zod';
import type { ApiFetcherData } from '~contract';

export type ValidResult<T extends { status: number }> = T extends { status: StatusCodes.OK } ? T : never;
export type InvalidResult<T extends { status: number }> = Exclude<T, { status: StatusCodes.OK }>;

export function assertTsRestResultOK<T extends { status: number }>(
export function assertTsRestResultOK<T extends ApiFetcherData>(
result: T,
errorArgs?: (result: InvalidResult<T>) => Parameters<typeof error>,
): asserts result is ValidResult<T> {
if (result.status !== StatusCodes.OK) {
const definedErrorArgs = errorArgs?.(result as Exclude<T, { status: StatusCodes.OK }>) ?? [result.status as never];
const definedErrorArgs: Parameters<typeof error> = errorArgs?.(result as Exclude<T, { status: StatusCodes.OK }>) ?? [
result.status as never,
(() => {
const message = extractErrorMessageFromApiFetcherData(result);

if (message instanceof Promise) {
// This should never happen, only when the body is a blob which shouldn't be an issue here
throw new Error('Cannot handle promise error messages');
}

return message;
})(),
];

error(...definedErrorArgs);
}
Expand Down Expand Up @@ -47,7 +61,7 @@ export type AssertTsRestActionResultOKArgs<T extends { status: number }> = {
onNotOk?: (result: InvalidResult<T>, data: ActionNotValidData) => Awaitable<unknown>;
} & ({ toast?: Partial<ToastManagerData> } | { layoutAlert: Partial<LayoutAlertData> });

export function assertTsRestActionResultOK<T extends { status: number; body: unknown }>(args: AssertTsRestActionResultOKArgs<T>) {
export function assertTsRestActionResultOK<T extends ApiFetcherData>(args: AssertTsRestActionResultOKArgs<T>) {
// Define checking result function
const checkValidResult = async () => {
const result = await args.result();
Expand Down
8 changes: 7 additions & 1 deletion client/src/lib/utils/urls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@ export function reconstructUrl(url: URL, searchParams?: URLSearchParams) {

const paramsString = params.size > 0 ? `?${params.toString()}` : '';

return `${url.origin}${url.pathname}${paramsString}`;
return `${url.pathname}${paramsString}`;
}

export function reconstructUrlWithOrigin(url: URL, searchParams?: URLSearchParams) {
const path = reconstructUrl(url, searchParams);

return `${url.origin}${path}`;
}

export function stringParamsFromUrl(url: URL, ...keys: string[]) {
Expand Down
2 changes: 1 addition & 1 deletion client/src/routes/(auth)/register/+page.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const load = (async ({ url, locals: { tsrest } }) => {

const result = await tsrest.auth.initRegistration({ query: { registerToken } });

assertTsRestResultOK(result, (result) => [result.status, result.body.message]);
assertTsRestResultOK(result);

const { email, ...attributes } = result.body;

Expand Down
6 changes: 5 additions & 1 deletion client/src/routes/+error.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,9 @@
<span>{statusDetail !== statusDetailsKey ? statusDetail : $t(`common.errorpage.types.server.details.default`)}</span>
</PageError>
{:else}
<PageError icon="i-mdi-emoticon-sad-outline" errorMessage={$page.error?.message ?? 'Unknown error'}></PageError>
<PageError
icon="i-mdi-emoticon-sad-outline"
errorMessage={$page.error?.message ?? 'Unknown error'}
i18nPayload={$page.error?.i18nPayload}
/>
{/if}

0 comments on commit 8fb9300

Please sign in to comment.