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

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

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 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
4 changes: 0 additions & 4 deletions client/src/www/app/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import {Clipboard} from './clipboard';
import {EnvironmentVariables} from './environment';
import {localizeErrorCode} from './error_localizer';

Check failure on line 20 in client/src/www/app/app.ts

View workflow job for this annotation

GitHub Actions / Lint

'localizeErrorCode' is defined but never used. Allowed unused vars must match /^_/u

Check failure on line 20 in client/src/www/app/app.ts

View workflow job for this annotation

GitHub Actions / Lint

'localizeErrorCode' is defined but never used. Allowed unused vars must match /^_/u
import * as config from './outline_server_repository/config';
import {Settings, SettingsKey} from './settings';
import {Updater} from './updater';
Expand Down Expand Up @@ -330,10 +330,6 @@
buttonHandler = () => {
this.showErrorDetailsDialog(error.details);
};
} else if (error instanceof PlatformError) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in showErrorCauseDialog.

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

Copy link
Contributor

@sbruens sbruens Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We localize the errors in App.showLocalizedError:

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

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

toastMessage = localizeErrorCode(error.code, this.localize);
buttonMessage = this.localize('error-details');
buttonHandler = () => this.showErrorDetailsDialog(error.toString());
} else {
const hasErrorDetails = Boolean(error.message || error.cause);
toastMessage = this.localize('error-unexpected');
Expand Down
34 changes: 0 additions & 34 deletions client/src/www/app/error_localizer.ts

This file was deleted.

14 changes: 5 additions & 9 deletions client/src/www/app/main.cordova.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,11 @@ import {installDefaultMethodChannel, MethodChannel} from './method_channel';
import {VpnApi} from './outline_server_repository/vpn';
import {CordovaVpnApi} from './outline_server_repository/vpn.cordova';
import {OutlinePlatform} from './platform';
import {
OUTLINE_PLUGIN_NAME,
pluginExec,
pluginExecWithErrorCode,
} from './plugin.cordova';
import {OUTLINE_PLUGIN_NAME, pluginExec} from './plugin.cordova';
import {AbstractUpdater} from './updater';
import * as interceptors from './url_interceptor';
import {NoOpVpnInstaller, VpnInstaller} from './vpn_installer';
import {PlatformError} from '../model/platform_error';
import {ipcToAppError} from '../model/platform_error';
import {SentryErrorReporter, Tags} from '../shared/error_reporter';

const hasDeviceSupport = cordova.platformId !== 'browser';
Expand Down Expand Up @@ -76,12 +72,12 @@ class CordovaErrorReporter extends SentryErrorReporter {
}

class CordovaMethodChannel implements MethodChannel {
invokeMethod(methodName: string, params: string): Promise<string> {
async invokeMethod(methodName: string, params: string): Promise<string> {
try {
return pluginExecWithErrorCode('invokeMethod', methodName, params);
return await pluginExec('invokeMethod', methodName, params);
} catch (e) {
console.debug('invokeMethod failed', methodName, e);
throw PlatformError.parseFrom(e);
throw ipcToAppError(e);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions client/src/www/app/main.electron.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {AbstractUpdater} from './updater';
import {UrlInterceptor} from './url_interceptor';
import {VpnInstaller} from './vpn_installer';
import {ErrorCode, OutlinePluginError} from '../model/errors';
import {PlatformError} from '../model/platform_error';
import {ipcToAppError} from '../model/platform_error';
import {
getSentryBrowserIntegrations,
OutlineErrorReporter,
Expand Down Expand Up @@ -137,7 +137,7 @@ class ElectronMethodChannel implements MethodChannel {
params
);
} catch (e) {
throw PlatformError.parseFrom(e);
throw ipcToAppError(e);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions client/src/www/app/plugin.cordova.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {PlatformError} from '../model/platform_error';
import {ipcToAppError} from '../model/platform_error';

export const OUTLINE_PLUGIN_NAME = 'OutlinePlugin';

Expand All @@ -30,6 +30,6 @@ export async function pluginExecWithErrorCode<T>(
try {
return await pluginExec<T>(cmd, ...args);
} catch (e) {
throw PlatformError.parseFrom(e);
throw ipcToAppError(e);
}
}
75 changes: 34 additions & 41 deletions client/src/www/model/platform_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@

import {CustomError} from '@outline/infrastructure/custom_error';

import * as errors from './errors';

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* @fileoverview This file defines types and constants corresponding to the backend Go's
* `platerrors` package. It will be used to receive native errors from Go, eventually replacing
Expand Down Expand Up @@ -80,35 +86,6 @@ function convertRawErrorObjectToPlatformError(rawObj: object): PlatformError {
return new PlatformError(code, rawObj.message, options);
}

/**
* Recursively converts a {@link PlatformError} into a raw JavaScript object that
* could be converted into a JSON string.
* @param {PlatformError} platErr Any non-null PlatformError.
* @returns {object} A plain JavaScript object that can be converted to JSON.
*/
function convertPlatformErrorToRawErrorObject(platErr: PlatformError): object {
const rawObj: {
code: string;
message: string;
details?: ErrorDetails;
cause?: object;
} = {
code: platErr.code,
message: platErr.message,
details: platErr.details,
};
if (platErr.cause) {
let cause: PlatformError;
if (platErr.cause instanceof PlatformError) {
cause = platErr.cause;
} else {
cause = new PlatformError(INTERNAL_ERROR, String(platErr.cause));
}
rawObj.cause = convertPlatformErrorToRawErrorObject(cause);
}
return rawObj;
}

/**
* ErrorDetails represents the details map of a {@link PlatformError}.
* The keys in this map are strings, and the values can be of any data type.
Expand Down Expand Up @@ -216,14 +193,31 @@ export class PlatformError extends CustomError {
}
return result;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const errRawObj = convertPlatformErrorToRawErrorObject(this);
return JSON.stringify(errRawObj);
/**
* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseAppErrorFromIPC would be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jyyi1 marked this conversation as resolved.
Show resolved Hide resolved
const platError = convertRawErrorObjectToPlatformError(
JSON.parse(ipcError.message)
);
let options = undefined as {cause?: Error};
jyyi1 marked this conversation as resolved.
Show resolved Hide resolved
if (platError.cause instanceof Error) {
options = {cause: platError.cause};
}
switch (platError.code) {
case FETCH_CONFIG_FAILED:
return new errors.SessionConfigFetchFailed(platError.message, options);
case ILLEGAL_CONFIG:
return new errors.ServerAccessKeyInvalid(platError.message, options);
case PROXY_SERVER_UNREACHABLE:
return new errors.ServerUnreachable(platError.message, options);
case VPN_PERMISSION_NOT_GRANTED:
return new errors.VpnPermissionNotGranted(platError.message, options);
default:
return platError;
}
}

Expand All @@ -240,13 +234,12 @@ export type ErrorCode = string;

export const INTERNAL_ERROR: ErrorCode = 'ERR_INTERNAL_ERROR';

export const FETCH_CONFIG_FAILED: ErrorCode = 'ERR_FETCH_CONFIG_FAILURE';
export const ILLEGAL_CONFIG: ErrorCode = 'ERR_ILLEGAL_CONFIG';
const FETCH_CONFIG_FAILED: ErrorCode = 'ERR_FETCH_CONFIG_FAILURE';
daniellacosse marked this conversation as resolved.
Show resolved Hide resolved
const ILLEGAL_CONFIG: ErrorCode = 'ERR_ILLEGAL_CONFIG';

export const VPN_PERMISSION_NOT_GRANTED = 'ERR_VPN_PERMISSION_NOT_GRANTED';
const VPN_PERMISSION_NOT_GRANTED = 'ERR_VPN_PERMISSION_NOT_GRANTED';

export const PROXY_SERVER_UNREACHABLE: ErrorCode =
'ERR_PROXY_SERVER_UNREACHABLE';
const PROXY_SERVER_UNREACHABLE: ErrorCode = 'ERR_PROXY_SERVER_UNREACHABLE';

/** Indicates that the OS routing service is not running (electron only). */
export const ROUTING_SERVICE_NOT_RUNNING = 'ERR_ROUTING_SERVICE_NOT_RUNNING';
Loading