Replace newTranslatableError with UserFriendlyError (#10440

* Introduce UserFriendlyError * Replace newTranslatableError with UserFriendlyError * Remove ITranslatableError * Fix up some strict lints * Document when we/why we can remove * Update matrix-web-i18n Includes changes to find `new UserFriendlyError`, see matrix-org/matrix-web-i18n#6 * Include room ID in error * Translate fallback error * Translate better * Update i18n strings * Better re-use * Minor comment fixes
matrix-org · Mar 31, 2023 · ff1468b · ff1468b
1 parent 567248d
commit ff1468b
Show file tree

Hide file tree

Showing 10 changed files with 282 additions and 96 deletions.
diff --git a/package.json b/package.json
@@ -28,7 +28,7 @@
     "matrix_lib_main": "./lib/index.ts",
     "matrix_lib_typings": "./lib/index.d.ts",
     "matrix_i18n_extra_translation_funcs": [
-        "newTranslatableError"
+        "UserFriendlyError"
     ],
     "scripts": {
         "prepublishOnly": "yarn build",
@@ -203,7 +203,7 @@
         "jest-mock": "^29.2.2",
         "jest-raw-loader": "^1.0.1",
         "matrix-mock-request": "^2.5.0",
-        "matrix-web-i18n": "^1.3.0",
+        "matrix-web-i18n": "^1.4.0",
         "mocha-junit-reporter": "^2.2.0",
         "node-fetch": "2",
         "postcss-scss": "^4.0.4",

diff --git a/src/@types/global.d.ts b/src/@types/global.d.ts
@@ -187,6 +187,11 @@ declare global {
     }
 
     interface Error {
+        // Standard
+        // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
+        cause?: unknown;
+
+        // Non-standard
         // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/fileName
         fileName?: string;
         // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/lineNumber
@@ -195,6 +200,22 @@ declare global {
         columnNumber?: number;
     }
 
+    // We can remove these pieces if we ever update to `target: "es2022"` in our
+    // TypeScript config which supports the new `cause` property, see
+    // https://github.com/vector-im/element-web/issues/24913
+    interface ErrorOptions {
+        // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
+        cause?: unknown;
+    }
+
+    interface ErrorConstructor {
+        new (message?: string, options?: ErrorOptions): Error;
+        (message?: string, options?: ErrorOptions): Error;
+    }
+
+    // eslint-disable-next-line no-var
+    var Error: ErrorConstructor;
+
     // https://github.com/microsoft/TypeScript/issues/28308#issuecomment-650802278
     interface AudioWorkletProcessor {
         readonly port: MessagePort;

diff --git a/src/SlashCommands.tsx b/src/SlashCommands.tsx
diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx
@@ -34,7 +34,7 @@ import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo";
 
 import dis from "../../../dispatcher/dispatcher";
 import Modal from "../../../Modal";
-import { _t } from "../../../languageHandler";
+import { _t, UserFriendlyError } from "../../../languageHandler";
 import DMRoomMap from "../../../utils/DMRoomMap";
 import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton";
 import SdkConfig from "../../../SdkConfig";
@@ -448,7 +448,15 @@ export const UserOptionsSection: React.FC<{
                     const inviter = new MultiInviter(roomId || "");
                     await inviter.invite([member.userId]).then(() => {
                         if (inviter.getCompletionState(member.userId) !== "invited") {
-                            throw new Error(inviter.getErrorText(member.userId) ?? undefined);
+                            const errorStringFromInviterUtility = inviter.getErrorText(member.userId);
+                            if (errorStringFromInviterUtility) {
+                                throw new Error(errorStringFromInviterUtility);
+                            } else {
+                                throw new UserFriendlyError(
+                                    `User (%(user)s) did not end up as invited to %(roomId)s but no error was given from the inviter utility`,
+                                    { user: member.userId, roomId, cause: undefined },
+                                );
+                            }
                         }
                     });
                 } catch (err) {

diff --git a/src/editor/commands.tsx b/src/editor/commands.tsx
@@ -21,7 +21,7 @@ import { IContent } from "matrix-js-sdk/src/models/event";
 import EditorModel from "./model";
 import { Type } from "./parts";
 import { Command, CommandCategories, getCommand } from "../SlashCommands";
-import { ITranslatableError, _t, _td } from "../languageHandler";
+import { UserFriendlyError, _t, _td } from "../languageHandler";
 import Modal from "../Modal";
 import ErrorDialog from "../components/views/dialogs/ErrorDialog";
 import QuestionDialog from "../components/views/dialogs/QuestionDialog";
@@ -65,7 +65,7 @@ export async function runSlashCommand(
 ): Promise<[content: IContent | null, success: boolean]> {
     const result = cmd.run(roomId, threadId, args);
     let messageContent: IContent | null = null;
-    let error = result.error;
+    let error: any = result.error;
     if (result.promise) {
         try {
             if (cmd.category === CommandCategories.messages || cmd.category === CommandCategories.effects) {
@@ -86,9 +86,8 @@ export async function runSlashCommand(
         let errText;
         if (typeof error === "string") {
             errText = error;
-        } else if ((error as ITranslatableError).translatedMessage) {
-            // Check for translatable errors (newTranslatableError)
-            errText = (error as ITranslatableError).translatedMessage;
+        } else if (error instanceof UserFriendlyError) {
+            errText = error.translatedMessage;
         } else if (error.message) {
             errText = error.message;
         } else {

diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -435,6 +435,7 @@
     "Use an identity server to invite by email. Click continue to use the default identity server (%(defaultIdentityServerName)s) or manage in Settings.": "Use an identity server to invite by email. Click continue to use the default identity server (%(defaultIdentityServerName)s) or manage in Settings.",
     "Continue": "Continue",
     "Use an identity server to invite by email. Manage in Settings.": "Use an identity server to invite by email. Manage in Settings.",
+    "User (%(user)s) did not end up as invited to %(roomId)s but no error was given from the inviter utility": "User (%(user)s) did not end up as invited to %(roomId)s but no error was given from the inviter utility",
     "Joins room with given address": "Joins room with given address",
     "Leave room": "Leave room",
     "Unrecognised room address: %(roomAlias)s": "Unrecognised room address: %(roomAlias)s",

diff --git a/src/languageHandler.tsx b/src/languageHandler.tsx
@@ -46,21 +46,49 @@ counterpart.setSeparator("|");
 const FALLBACK_LOCALE = "en";
 counterpart.setFallbackLocale(FALLBACK_LOCALE);
 
-export interface ITranslatableError extends Error {
-    translatedMessage: string;
+interface ErrorOptions {
+    // Because we're mixing the subsitution variables and `cause` into the same object
+    // below, we want them to always explicitly say whether there is an underlying error
+    // or not to avoid typos of "cause" slipping through unnoticed.
+    cause: unknown | undefined;
 }
 
 /**
- * Helper function to create an error which has an English message
- * with a translatedMessage property for use by the consumer.
- * @param {string} message Message to translate.
- * @param {object} variables Variable substitutions, e.g { foo: 'bar' }
- * @returns {Error} The constructed error.
+ * Used to rethrow an error with a user-friendly translatable message while maintaining
+ * access to that original underlying error. Downstream consumers can display the
+ * `translatedMessage` property in the UI and inspect the underlying error with the
+ * `cause` property.
+ *
+ * The error message will display as English in the console and logs so Element
+ * developers can easily understand the error and find the source in the code. It also
+ * helps tools like Sentry deduplicate the error, or just generally searching in
+ * rageshakes to find all instances regardless of the users locale.
+ *
+ * @param message - The untranslated error message text, e.g "Something went wrong with %(foo)s".
+ * @param substitutionVariablesAndCause - Variable substitutions for the translation and
+ * original cause of the error. If there is no cause, just pass `undefined`, e.g { foo:
+ * 'bar', cause: err || undefined }
  */
-export function newTranslatableError(message: string, variables?: IVariables): ITranslatableError {
-    const error = new Error(message) as ITranslatableError;
-    error.translatedMessage = _t(message, variables);
-    return error;
+export class UserFriendlyError extends Error {
+    public readonly translatedMessage: string;
+
+    public constructor(message: string, substitutionVariablesAndCause?: IVariables & ErrorOptions) {
+        const errorOptions = {
+            cause: substitutionVariablesAndCause?.cause,
+        };
+        // Prevent "Could not find /%\(cause\)s/g in x" logs to the console by removing
+        // it from the list
+        const substitutionVariables = { ...substitutionVariablesAndCause };
+        delete substitutionVariables["cause"];
+
+        // Create the error with the English version of the message that we want to show
+        // up in the logs
+        const englishTranslatedMessage = _t(message, { ...substitutionVariables, locale: "en" });
+        super(englishTranslatedMessage, errorOptions);
+
+        // Also provide a translated version of the error in the users locale to display
+        this.translatedMessage = _t(message, substitutionVariables);
+    }
 }
 
 export function getUserLanguage(): string {
@@ -373,12 +401,18 @@ export function replaceByRegexes(text: string, mapping: IVariables | Tags): stri
             }
         }
         if (!matchFoundSomewhere) {
-            // The current regexp did not match anything in the input
-            // Missing matches is entirely possible because you might choose to show some variables only in the case
-            // of e.g. plurals. It's still a bit suspicious, and could be due to an error, so log it.
-            // However, not showing count is so common that it's not worth logging. And other commonly unused variables
-            // here, if there are any.
-            if (regexpString !== "%\\(count\\)s") {
+            if (
+                // The current regexp did not match anything in the input. Missing
+                // matches is entirely possible because you might choose to show some
+                // variables only in the case of e.g. plurals. It's still a bit
+                // suspicious, and could be due to an error, so log it. However, not
+                // showing count is so common that it's not worth logging. And other
+                // commonly unused variables here, if there are any.
+                regexpString !== "%\\(count\\)s" &&
+                // Ignore the `locale` option which can be used to override the locale
+                // in counterpart
+                regexpString !== "%\\(locale\\)s"
+            ) {
                 logger.log(`Could not find ${regexp} in ${text}`);
             }
         }
@@ -652,7 +686,11 @@ function doRegisterTranslations(customTranslations: ICustomTranslations): void {
  * This function should be called *after* registering other translations data to
  * ensure it overrides strings properly.
  */
-export async function registerCustomTranslations(): Promise<void> {
+export async function registerCustomTranslations({
+    testOnlyIgnoreCustomTranslationsCache = false,
+}: {
+    testOnlyIgnoreCustomTranslationsCache?: boolean;
+} = {}): Promise<void> {
     const moduleTranslations = ModuleRunner.instance.allTranslations;
     doRegisterTranslations(moduleTranslations);
 
@@ -661,7 +699,7 @@ export async function registerCustomTranslations(): Promise<void> {
 
     try {
         let json: Optional<ICustomTranslations>;
-        if (Date.now() >= cachedCustomTranslationsExpire) {
+        if (testOnlyIgnoreCustomTranslationsCache || Date.now() >= cachedCustomTranslationsExpire) {
             json = CustomTranslationOptions.lookupFn
                 ? CustomTranslationOptions.lookupFn(lookupUrl)
                 : ((await (await fetch(lookupUrl)).json()) as ICustomTranslations);

diff --git a/src/utils/AutoDiscoveryUtils.tsx b/src/utils/AutoDiscoveryUtils.tsx
@@ -19,7 +19,7 @@ import { AutoDiscovery, ClientConfig } from "matrix-js-sdk/src/autodiscovery";
 import { logger } from "matrix-js-sdk/src/logger";
 import { IClientWellKnown } from "matrix-js-sdk/src/matrix";
 
-import { _t, _td, newTranslatableError } from "../languageHandler";
+import { _t, UserFriendlyError } from "../languageHandler";
 import { makeType } from "./TypeUtils";
 import SdkConfig from "../SdkConfig";
 import { ValidatedServerConfig } from "./ValidatedServerConfig";
@@ -147,7 +147,7 @@ export default class AutoDiscoveryUtils {
         syntaxOnly = false,
     ): Promise<ValidatedServerConfig> {
         if (!homeserverUrl) {
-            throw newTranslatableError(_td("No homeserver URL provided"));
+            throw new UserFriendlyError("No homeserver URL provided");
         }
 
         const wellknownConfig: IClientWellKnown = {
@@ -199,7 +199,7 @@ export default class AutoDiscoveryUtils {
             // This shouldn't happen without major misconfiguration, so we'll log a bit of information
             // in the log so we can find this bit of codee but otherwise tell teh user "it broke".
             logger.error("Ended up in a state of not knowing which homeserver to connect to.");
-            throw newTranslatableError(_td("Unexpected error resolving homeserver configuration"));
+            throw new UserFriendlyError("Unexpected error resolving homeserver configuration");
         }
 
         const hsResult = discoveryResult["m.homeserver"];
@@ -221,9 +221,9 @@ export default class AutoDiscoveryUtils {
             logger.error("Error determining preferred identity server URL:", isResult);
             if (isResult.state === AutoDiscovery.FAIL_ERROR) {
                 if (AutoDiscovery.ALL_ERRORS.indexOf(isResult.error as string) !== -1) {
-                    throw newTranslatableError(isResult.error as string);
+                    throw new UserFriendlyError(String(isResult.error));
                 }
-                throw newTranslatableError(_td("Unexpected error resolving identity server configuration"));
+                throw new UserFriendlyError("Unexpected error resolving identity server configuration");
             } // else the error is not related to syntax - continue anyways.
 
             // rewrite homeserver error since we don't care about problems
@@ -237,9 +237,9 @@ export default class AutoDiscoveryUtils {
             logger.error("Error processing homeserver config:", hsResult);
             if (!syntaxOnly || !AutoDiscoveryUtils.isLivelinessError(hsResult.error)) {
                 if (AutoDiscovery.ALL_ERRORS.indexOf(hsResult.error as string) !== -1) {
-                    throw newTranslatableError(hsResult.error as string);
+                    throw new UserFriendlyError(String(hsResult.error));
                 }
-                throw newTranslatableError(_td("Unexpected error resolving homeserver configuration"));
+                throw new UserFriendlyError("Unexpected error resolving homeserver configuration");
             } // else the error is not related to syntax - continue anyways.
         }
 
@@ -252,7 +252,7 @@ export default class AutoDiscoveryUtils {
         // It should have been set by now, so check it
         if (!preferredHomeserverName) {
             logger.error("Failed to parse homeserver name from homeserver URL");
-            throw newTranslatableError(_td("Unexpected error resolving homeserver configuration"));
+            throw new UserFriendlyError("Unexpected error resolving homeserver configuration");
         }
 
         return makeType(ValidatedServerConfig, {