chore(client): use application errors instead of PlatformErrors #2337

fortuna · 2025-01-22T00:19:13Z

convertRawErrorObjectToPlatformError is now a place where we can convert from the PlatformError serialized errors to application errors.

Note how it deletes a significant amount of code. We were representing the same errors multiple times.

Before this PR, deserialization had to happen at different places in the code, complicating application code and making it impractical to change serialization. Error deserialization is now centralized, and we need to move away from using PlatformError in application code.

The motivation for this PR was propagating the Provider error from the dynamic key. It has a message and a detailed message, and our application error models exactly that. With PlatformError I will have to create my own schema to encode that in the error, but then the application code had to be aware of that schema as well in order to use the messages correctly, which was problematic.

jyyi1

We included arbitrary details in the errors, we should forward them to the strongly typed errors as well.

jyyi1 · 2025-01-22T00:24:57Z

client/src/www/model/platform_error.ts

+ * ipcToAppError converts an Error returned from the MethodChannel IPC to an application error.
+ * MethodChannel errors encode its information as a JSON object in the Error message.
+ */
+export function ipcToAppError(ipcError: Error): CustomError {


parseAppErrorFromIPC would be better?

I'm not sure about using "parse" here, since the input is an object, not serialized text.

jyyi1 · 2025-01-22T00:27:14Z

client/src/www/app/app.ts

@@ -330,10 +330,6 @@ export class App {
      buttonHandler = () => {
        this.showErrorDetailsDialog(error.details);
      };
-    } else if (error instanceof PlatformError) {


I would keep this. The default handler (showErrorCauseDialog) will print weird outputs (for example, duplicated causes). PlatformError's toString already handles the format and prints a cause chain.

Also the default handler doesn't print out error details Map.

Fixed in showErrorCauseDialog.

We shouldn't be using localizeErrorCode anymore, it's not needed.

We shouldn't be using localizeErrorCode anymore, it's not needed.

Where is localization of errors happening now? Are we no longer localizing these errors and just throwing a generic localized "unknown error" for them? That's a regression and is a worse user experience. We should localize the top-level user-facing error that can tell the user what went wrong (like the one suggesting to contact their service provider with details).

We localize the errors in App.showLocalizedError:

outline-apps/client/src/www/app/app.ts

Line 246 in 2e3bd3d

showLocalizedError(error?: Error, toastDuration = 10000) {

I'm actually removing the duplicate logic in this PR.

jyyi1 · 2025-01-22T00:32:39Z

client/src/www/model/platform_error.ts

-   * Returns a JSON string of this error with all details and causes.
-   * @returns {string} The JSON string representing this error.
-   */
-  toJSON(): string {


This is used when initiating a PlatformError from electron main process (not from Go) to renderer process: https://github.com/Jigsaw-Code/outline-apps/blob/master/client/electron/routing_service.ts#L141

Fixed.

Ugh... Perhaps we can move the error serialization to Go at some point.

Yes, we should move the logic to Go in the future. All errors should be triggered from Go.

client/src/www/model/platform_error.ts

fortuna

Another look?

fortuna · 2025-01-22T20:57:52Z

client/src/www/model/platform_error.ts

+ * ipcToAppError converts an Error returned from the MethodChannel IPC to an application error.
+ * MethodChannel errors encode its information as a JSON object in the Error message.
+ */
+export function ipcToAppError(ipcError: Error): CustomError {


I'm not sure about using "parse" here, since the input is an object, not serialized text.

client/src/www/model/platform_error.ts

fortuna · 2025-01-22T22:12:16Z

client/src/www/model/platform_error.ts

-   * Returns a JSON string of this error with all details and causes.
-   * @returns {string} The JSON string representing this error.
-   */
-  toJSON(): string {


Fixed.

Ugh... Perhaps we can move the error serialization to Go at some point.

client/src/www/model/platform_error.ts

fortuna · 2025-01-22T23:21:30Z

client/src/www/app/app.ts

@@ -330,10 +330,6 @@ export class App {
      buttonHandler = () => {
        this.showErrorDetailsDialog(error.details);
      };
-    } else if (error instanceof PlatformError) {


Fixed in showErrorCauseDialog.

We shouldn't be using localizeErrorCode anymore, it's not needed.

jyyi1

Thanks for the update. Added some comments.

jyyi1 · 2025-01-23T21:46:26Z

client/go/outline/platerrors/error_code.go

-	// FetchConfigFailed means we failed to fetch a config from a remote location.
-	FetchConfigFailed ErrorCode = "ERR_FETCH_CONFIG_FAILURE"
+	// GenericErr indicates a generic error that is not directly surfaced to the user.
+	GenericErr ErrorCode = "ERR_GENERIC"


Can we use the InternalError ErrorCode = "ERR_INTERNAL_ERROR" defined above, which has already been widely used across the code base?

jyyi1 · 2025-01-23T21:52:03Z

client/src/www/app/app.ts

@@ -756,7 +750,7 @@ export class App {
  private showErrorCauseDialog(error: Error) {
    let message = error.toString();

-    if (error.cause) {
+    if (!(error instanceof PlatformError && error.cause)) {


Correct me if I was wrong: if (!(error instanceof PlatformError && error.cause)) <=> if ((error not-instanceof PlatformError) or (!error.cause)), then we show the cause. Is this correct?

jyyi1 · 2025-01-23T21:55:31Z

client/src/www/model/errors.ts

-  constructor(message: string, options?: {cause?: Error}) {
-    super(message, options);
+  constructor(cause?: Error) {
+    super(undefined, {cause});


The undefined message seems to be off from the best practice of JavaScript's Error object:

The message data property of an Error instance is a human-readable description of the error.

Maybe we can have a default human-readable message?

jyyi1 · 2025-01-23T22:01:20Z

client/src/www/model/platform_error.ts

+
+// TODO(fortuna): Remove PlatformError from the model.
+// PlatformError is an implementation detail. It does not belong in the model.
+// It's also about serialization, we should not use it in application code.


👍 As we have a unified MethodChannel now, we can convert the errors there, without exporting any internal implementations.

jyyi1 · 2025-01-23T22:03:09Z

client/go/outline/fetch.go

@@ -45,7 +45,7 @@ func fetchResource(url string) (string, error) {
 	resp.Body.Close()
 	if resp.StatusCode > 299 {
 		return "", platerrors.PlatformError{
-			Code:    platerrors.FetchConfigFailed,
+			Code:    platerrors.GenericErr,


It might be useful to expose this error details? Especially the HTTP response code?

fortuna · 2025-01-24T18:51:54Z

Closing in favor of the simpler #2346

fortuna added 2 commits January 21, 2025 18:13

Introduce ipcToAppError

fa5dca4

Clean up

ac9d754

fortuna requested a review from jyyi1 January 22, 2025 00:19

fortuna requested a review from a team as a code owner January 22, 2025 00:19

github-actions bot added the size/M label Jan 22, 2025

fortuna requested a review from sbruens January 22, 2025 00:19

fortuna added 3 commits January 21, 2025 19:30

Improve

726dd2e

Add tODO

8cfdfd4

Revert

7c0fb81

github-actions bot added size/S and removed size/M labels Jan 22, 2025

Remove import

530eceb

jyyi1 reviewed Jan 22, 2025

View reviewed changes

daniellacosse approved these changes Jan 22, 2025

View reviewed changes

client/src/www/model/platform_error.ts Outdated Show resolved Hide resolved

fortuna mentioned this pull request Jan 22, 2025

Add support for Websocket endpoints #2272

Open

Updates

bc433ed

fortuna requested a review from a team as a code owner January 22, 2025 22:31

github-actions bot added size/M and removed size/S labels Jan 22, 2025

More cleanup

edac0c2

fortuna commented Jan 22, 2025

View reviewed changes

fortuna requested a review from jyyi1 January 22, 2025 23:21

fortuna added 2 commits January 22, 2025 18:23

Fixes

9457111

fix linux

d27c5b1

jyyi1 reviewed Jan 23, 2025

View reviewed changes

Fudge

465e6b1

github-actions bot added size/L and removed size/M labels Jan 23, 2025

fortuna mentioned this pull request Jan 24, 2025

chore(client): clean up errors #2346

Merged

fortuna closed this Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(client): use application errors instead of PlatformErrors #2337

chore(client): use application errors instead of PlatformErrors #2337

fortuna commented Jan 22, 2025 •

edited

Loading

jyyi1 left a comment •

edited

Loading

jyyi1 Jan 22, 2025

fortuna Jan 22, 2025

jyyi1 Jan 22, 2025

fortuna Jan 22, 2025

sbruens Jan 23, 2025 •

edited

Loading

fortuna Jan 23, 2025

jyyi1 Jan 22, 2025

fortuna Jan 22, 2025

jyyi1 Jan 23, 2025

fortuna left a comment

fortuna Jan 22, 2025

fortuna Jan 22, 2025

fortuna Jan 22, 2025

jyyi1 left a comment

jyyi1 Jan 23, 2025 •

edited

Loading

jyyi1 Jan 23, 2025

jyyi1 Jan 23, 2025

jyyi1 Jan 23, 2025

jyyi1 Jan 23, 2025

fortuna commented Jan 24, 2025

chore(client): use application errors instead of PlatformErrors #2337

chore(client): use application errors instead of PlatformErrors #2337

Conversation

fortuna commented Jan 22, 2025 • edited Loading

jyyi1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbruens Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

jyyi1 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna commented Jan 24, 2025

fortuna commented Jan 22, 2025 •

edited

Loading

jyyi1 left a comment •

edited

Loading

sbruens Jan 23, 2025 •

edited

Loading

jyyi1 Jan 23, 2025 •

edited

Loading