From 1a46ac030130a85e4fc02429f1b61e953ddb93ca Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Wed, 3 Aug 2016 13:29:23 -0400 Subject: [PATCH] refactor(GUI): unify error handling (#620) Error handling is currently a mess. The knowledge to correctly report an error both to the end user and to us is scattered in many places. This PR introduces the following changes: - Rename `AnalyticsService.logDebug()` to `AnalyticsService.logDebug()` to clarify better the intention of the function. - Move `$log` decorators from an `AnalyticsService` run block to `AnalyticsService.logDebug()`. - Implement `AnalyticsService.logException()`, whose duty is to log an exception to TrackJS or any related service, and log it to DevTools. - Implement `ErrorService.reportException()`, whose duty is to report an exception to every interested party. This means logging the error to TrackJS, displaying it DevTools and showing a nice alert to the user. This function is based from `handleError()` from `MainController`. - Move global `$exceptionHandler` error handler to the entry point of the application, and make it simply call `ErrorService.reportException()`. - Replace every `handleError()` call in `MainController` with `ErrorService.reportException()`. Signed-off-by: Juan Cruz Viotti --- lib/gui/app.js | 13 ++++- lib/gui/modules/analytics.js | 70 ++++++++++++-------------- lib/gui/modules/error.js | 58 +++++++++++++++++++++ lib/gui/pages/main/controllers/main.js | 25 ++------- lib/gui/pages/main/main.js | 1 + 5 files changed, 109 insertions(+), 58 deletions(-) create mode 100644 lib/gui/modules/error.js diff --git a/lib/gui/app.js b/lib/gui/app.js index 388fc7a5..7ebb4ccc 100644 --- a/lib/gui/app.js +++ b/lib/gui/app.js @@ -35,6 +35,7 @@ const app = angular.module('Etcher', [ // Etcher modules require('./modules/analytics'), + require('./modules/error'), // Models require('./models/selection-state'), @@ -95,7 +96,7 @@ app.run((AnalyticsService, OSWindowProgressService, FlashStateModel) => { return; } - AnalyticsService.log([ + AnalyticsService.logDebug([ `Progress (${flashState.type}):`, `${flashState.percentage}% at ${flashState.speed} MB/s`, `(eta ${flashState.eta}s)` @@ -108,3 +109,13 @@ app.run((AnalyticsService, OSWindowProgressService, FlashStateModel) => { app.config(($urlRouterProvider) => { $urlRouterProvider.otherwise('/main'); }); + +app.config(($provide) => { + $provide.decorator('$exceptionHandler', ($delegate, $injector) => { + return (exception, cause) => { + const ErrorService = $injector.get('ErrorService'); + ErrorService.reportException(exception); + $delegate(exception, cause); + }; + }); +}); diff --git a/lib/gui/modules/analytics.js b/lib/gui/modules/analytics.js index b5ee8854..3b345835 100644 --- a/lib/gui/modules/analytics.js +++ b/lib/gui/modules/analytics.js @@ -39,6 +39,9 @@ const analytics = angular.module(MODULE_NAME, [ require('../models/settings') ]); +// Mixpanel integration +// https://github.com/kuhnza/angular-mixpanel + analytics.config(($mixpanelProvider) => { $mixpanelProvider.apiKey('63e5fc4563e00928da67d1226364dd4c'); @@ -74,41 +77,7 @@ analytics.run(($window) => { }); }); -analytics.config(($provide) => { - $provide.decorator('$exceptionHandler', ($delegate, $window, $injector) => { - return (exception, cause) => { - const SettingsModel = $injector.get('SettingsModel'); - - if (SettingsModel.get('errorReporting') && isRunningInAsar()) { - $window.trackJs.track(exception); - } - - $delegate(exception, cause); - }; - }); - - $provide.decorator('$log', ($delegate, $window, $injector) => { - - // Save the original $log.debug() - const debugFn = $delegate.debug; - - $delegate.debug = (message) => { - message = new Date() + ' ' + message; - - const SettingsModel = $injector.get('SettingsModel'); - - if (SettingsModel.get('errorReporting') && isRunningInAsar()) { - $window.trackJs.console.debug(message); - } - - debugFn(message); - }; - - return $delegate; - }); -}); - -analytics.service('AnalyticsService', function($log, $mixpanel, SettingsModel) { +analytics.service('AnalyticsService', function($log, $window, $mixpanel, SettingsModel) { /** * @summary Log a debug message @@ -123,7 +92,13 @@ analytics.service('AnalyticsService', function($log, $mixpanel, SettingsModel) { * @example * AnalyticsService.log('Hello World'); */ - this.log = (message) => { + this.logDebug = (message) => { + message = new Date() + ' ' + message; + + if (SettingsModel.get('errorReporting') && isRunningInAsar()) { + $window.trackJs.console.debug(message); + } + $log.debug(message); }; @@ -158,7 +133,28 @@ analytics.service('AnalyticsService', function($log, $mixpanel, SettingsModel) { message += ` (${JSON.stringify(data)})`; } - this.log(message); + this.logDebug(message); + }; + + /** + * @summary Log an exception + * @function + * @public + * + * @description + * This function logs an exception in TrackJS. + * + * @param {Error} exception - exception + * + * @example + * AnalyticsService.logException(new Error('Something happened')); + */ + this.logException = (exception) => { + if (SettingsModel.get('errorReporting') && isRunningInAsar()) { + $window.trackJs.track(exception); + } + + $log.error(exception); }; }); diff --git a/lib/gui/modules/error.js b/lib/gui/modules/error.js new file mode 100644 index 00000000..c6e08835 --- /dev/null +++ b/lib/gui/modules/error.js @@ -0,0 +1,58 @@ +/* + * Copyright 2016 Resin.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +/** + * @module Etcher.Modules.Error + */ + +const angular = require('angular'); + +const MODULE_NAME = 'Etcher.Modules.Error'; +const error = angular.module(MODULE_NAME, [ + require('../modules/analytics'), + require('../os/dialog/dialog') +]); + +error.service('ErrorService', function(AnalyticsService, OSDialogService) { + + /** + * @summary Report an exception + * @function + * @public + * + * @param {Error} exception - exception + * + * @example + * ErrorService.reportException(new Error('Something happened')); + */ + this.reportException = (exception) => { + + // This particular error is handled by the alert ribbon + // on the main application page. + if (exception.code === 'ENOSPC') { + AnalyticsService.logEvent('Drive ran out of space'); + return; + } + + OSDialogService.showError(exception, exception.description); + AnalyticsService.logException(exception); + }; + +}); + +module.exports = MODULE_NAME; diff --git a/lib/gui/pages/main/controllers/main.js b/lib/gui/pages/main/controllers/main.js index 8867d47d..5e983af9 100644 --- a/lib/gui/pages/main/controllers/main.js +++ b/lib/gui/pages/main/controllers/main.js @@ -29,6 +29,7 @@ module.exports = function( DrivesModel, ImageWriterService, AnalyticsService, + ErrorService, DriveSelectorService, TooltipModalService, OSWindowProgressService, @@ -44,25 +45,9 @@ module.exports = function( this.settings = SettingsModel; this.tooltipModal = TooltipModalService; - const handleError = (error) => { - - // This particular error is handled by the alert ribbon - // on the main application page. - if (error.code === 'ENOSPC') { - AnalyticsService.logEvent('Drive ran out of space'); - return; - } - - OSDialogService.showError(error, error.description); - - // Also throw it so it gets displayed in DevTools - // and its reported by TrackJS. - throw error; - }; - DriveScannerService.start(); - DriveScannerService.on('error', handleError); + DriveScannerService.on('error', ErrorService.reportException); DriveScannerService.on('drives', (drives) => { @@ -134,7 +119,7 @@ module.exports = function( } this.selectImage(image); - }).catch(handleError); + }).catch(ErrorService.reportException); }; this.selectDrive = (drive) => { @@ -152,7 +137,7 @@ module.exports = function( this.openDriveSelector = () => { DriveSelectorService.open() .then(this.selectDrive) - .catch(handleError); + .catch(ErrorService.reportException); }; this.reselectImage = () => { @@ -225,7 +210,7 @@ module.exports = function( AnalyticsService.logEvent('Flash error'); } - handleError(error); + ErrorService.reportException(error); }) .finally(OSWindowProgressService.clear); }; diff --git a/lib/gui/pages/main/main.js b/lib/gui/pages/main/main.js index c270a1d7..e819fbca 100644 --- a/lib/gui/pages/main/main.js +++ b/lib/gui/pages/main/main.js @@ -43,6 +43,7 @@ const MainPage = angular.module(MODULE_NAME, [ require('../../modules/drive-scanner'), require('../../modules/image-writer'), require('../../modules/analytics'), + require('../../modules/error'), require('../../models/selection-state'), require('../../models/flash-state'), require('../../models/settings'),