Split up application error handling (#3184)

Co-authored-by: David Wheatley <hi@davwheat.dev>
flarum · Dec 11, 2021 · 57b413a · 57b413a
1 parent 326b787
commit 57b413a
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 83 deletions.
diff --git a/js/src/common/Application.tsx b/js/src/common/Application.tsx
@@ -366,7 +366,7 @@ export default class Application {
   }
 
   protected transformRequestOptions<ResponseType>(flarumOptions: FlarumRequestOptions<ResponseType>): InternalFlarumRequestOptions<ResponseType> {
-    const { background, deserialize, errorHandler, extract, modifyText, ...tmpOptions } = { ...flarumOptions };
+    const { background, deserialize, extract, modifyText, ...tmpOptions } = { ...flarumOptions };
 
     // Unless specified otherwise, requests should run asynchronously in the
     // background, so that they don't prevent redraws from occurring.
@@ -380,10 +380,6 @@ export default class Application {
     // so it errors due to Mithril's typings
     const defaultDeserialize = (response: string) => response as ResponseType;
 
-    const defaultErrorHandler = (error: RequestError) => {
-      throw error;
-    };
-
     // When extracting the data from the response, we can check the server
     // response code and show an error message to the user if something's gone
     // awry.
@@ -392,7 +388,6 @@ export default class Application {
     const options: InternalFlarumRequestOptions<ResponseType> = {
       background: background ?? defaultBackground,
       deserialize: deserialize ?? defaultDeserialize,
-      errorHandler: errorHandler ?? defaultErrorHandler,
       ...tmpOptions,
     };
 
@@ -460,86 +455,90 @@ export default class Application {
 
     if (this.requestErrorAlert) this.alerts.dismiss(this.requestErrorAlert);
 
-    // Now make the request. If it's a failure, inspect the error that was
-    // returned and show an alert containing its contents.
-    return m.request(options).then(
-      (response) => response,
-      (error: RequestError) => {
-        let content;
-
-        switch (error.status) {
-          case 422:
-            content = ((error.response?.errors ?? {}) as Record<string, unknown>[])
-              .map((error) => [error.detail, <br />])
-              .flat()
-              .slice(0, -1);
-            break;
-
-          case 401:
-          case 403:
-            content = app.translator.trans('core.lib.error.permission_denied_message');
-            break;
-
-          case 404:
-          case 410:
-            content = app.translator.trans('core.lib.error.not_found_message');
-            break;
-
-          case 413:
-            content = app.translator.trans('core.lib.error.payload_too_large_message');
-            break;
-
-          case 429:
-            content = app.translator.trans('core.lib.error.rate_limit_exceeded_message');
-            break;
-
-          default:
-            content = app.translator.trans('core.lib.error.generic_message');
-        }
+    return m.request(options).catch((e) => this.requestErrorCatch(e, originalOptions.errorHandler));
+  }
 
-        const isDebug = app.forum.attribute('debug');
-        // contains a formatted errors if possible, response must be an JSON API array of errors
-        // the details property is decoded to transform escaped characters such as '\n'
-        const errors = error.response && error.response.errors;
-        const formattedError = (Array.isArray(errors) && errors?.[0]?.detail && errors.map((e) => decodeURI(e.detail ?? ''))) || undefined;
-
-        error.alert = {
-          type: 'error',
-          content,
-          controls: isDebug && [
-            <Button className="Button Button--link" onclick={this.showDebug.bind(this, error, formattedError)}>
-              {app.translator.trans('core.lib.debug_button')}
-            </Button>,
-          ],
-        };
-
-        try {
-          options.errorHandler(error);
-        } catch (error) {
-          if (error instanceof RequestError) {
-            if (isDebug && error.xhr) {
-              const { method, url } = error.options;
-              const { status = '' } = error.xhr;
-
-              console.group(`${method} ${url} ${status}`);
-
-              console.error(...(formattedError || [error]));
-
-              console.groupEnd();
-            }
-
-            this.requestErrorAlert = this.alerts.show(error.alert, error.alert.content);
-          } else {
-            throw error;
-          }
-        }
+  /**
+   * By default, show an error alert, and log the error to the console.
+   */
+  protected requestErrorCatch<ResponseType>(error: RequestError, customErrorHandler: FlarumRequestOptions<ResponseType>['errorHandler']) {
+    // the details property is decoded to transform escaped characters such as '\n'
+    const formattedErrors = error.response?.errors?.map((e) => decodeURI(e.detail ?? '')) ?? [];
+
+    let content;
+    switch (error.status) {
+      case 422:
+        content = formattedErrors
+          .map((detail) => [detail, <br />])
+          .flat()
+          .slice(0, -1);
+        break;
+
+      case 401:
+      case 403:
+        content = app.translator.trans('core.lib.error.permission_denied_message');
+        break;
+
+      case 404:
+      case 410:
+        content = app.translator.trans('core.lib.error.not_found_message');
+        break;
+
+      case 413:
+        content = app.translator.trans('core.lib.error.payload_too_large_message');
+        break;
+
+      case 429:
+        content = app.translator.trans('core.lib.error.rate_limit_exceeded_message');
+        break;
+
+      default:
+        content = app.translator.trans('core.lib.error.generic_message');
+    }
+
+    const isDebug: boolean = app.forum.attribute('debug');
+
+    error.alert = {
+      type: 'error',
+      content,
+      controls: isDebug && [
+        <Button className="Button Button--link" onclick={this.showDebug.bind(this, error, formattedErrors)}>
+          {app.translator.trans('core.lib.debug_button')}
+        </Button>,
+      ],
+    };
 
-        return Promise.reject(error);
+    if (customErrorHandler) {
+      customErrorHandler(error);
+    } else {
+      this.requestErrorDefaultHandler(error, isDebug, formattedErrors);
+    }
+
+    return Promise.reject(error);
+  }
+
+  protected requestErrorDefaultHandler(e: unknown, isDebug: boolean, formattedErrors: string[]): void {
+    if (e instanceof RequestError) {
+      if (isDebug && e.xhr) {
+        const { method, url } = e.options;
+        const { status = '' } = e.xhr;
+
+        console.group(`${method} ${url} ${status}`);
+
+        console.error(...(formattedErrors || [e]));
+
+        console.groupEnd();
       }
-    );
+
+      if (e.alert) {
+        this.requestErrorAlert = this.alerts.show(e.alert, e.alert.content);
+      }
+    } else {
+      throw e;
+    }
   }
 
-  private showDebug(error: RequestError, formattedError?: string[]) {
+  private showDebug(error: RequestError, formattedError: string[]) {
     if (this.requestErrorAlert !== null) this.alerts.dismiss(this.requestErrorAlert);
 
     this.modal.show(RequestErrorModal, { error, formattedError });

diff --git a/js/src/common/utils/RequestError.ts b/js/src/common/utils/RequestError.ts
@@ -1,7 +1,7 @@
 import type Mithril from 'mithril';
+import type { AlertAttrs } from '../components/Alert';
 
 export type InternalFlarumRequestOptions<ResponseType> = Mithril.RequestOptions<ResponseType> & {
-  errorHandler: (error: RequestError) => void;
   url: string;
 };
 
@@ -20,7 +20,7 @@ export default class RequestError<ResponseType = string> {
     }[];
   } | null;
 
-  alert: any;
+  alert: AlertAttrs | null;
 
   constructor(status: number, responseText: string | null, options: InternalFlarumRequestOptions<ResponseType>, xhr: XMLHttpRequest) {
     this.status = status;