From c4c8e2c038587142c81bb42d9211704953b63f45 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Wed, 22 Jun 2016 16:22:59 -0400 Subject: [PATCH] refactor: manage application state with redux (#508) Currently, Etcher's application state is stored among various models, making it a bit hard to mentually visualise the application state at a certain point of execution. This also means that some quirky bugs appear, and we have to take non-elegant measures to mitigate them, for example: - The current drive selection might be invalid if the current available drive list doesn't contain it anymore. - The progress state might be modified while there is no flashing in process. - We have to rely on event emission to propagate the current state to the application progress bar. - The validity of a selected drive might depend on the currently selected image. While all these issues can be addressed with common programming techniques, Redux introduces a new way of thinking about the application state that make the above problems non-existent, or trivial to fix. This PR creates a Redux store containing the logic used to mutate state from: - `SelectionStateModel`. - `DrivesMode`. - `ImageWriterService`. We are also making extra effort to preserve the public APIs from the models, which we will be simplifying in later commits. There is still much to be done, but we're happy to be taking the first steo towards a much cleaner architecture. Signed-off-by: Juan Cruz Viotti --- lib/gui/models/drives.js | 33 ++---- lib/gui/models/selection-state.js | 117 +++++-------------- lib/gui/models/store.js | 140 +++++++++++++++++++++++ lib/gui/modules/image-writer.js | 44 ++++--- package.json | 2 + tests/gui/models/selection-state.spec.js | 2 +- tests/gui/modules/image-writer.spec.js | 1 + 7 files changed, 211 insertions(+), 128 deletions(-) create mode 100644 lib/gui/models/store.js diff --git a/lib/gui/models/drives.js b/lib/gui/models/drives.js index 6cc69dff..fc287965 100644 --- a/lib/gui/models/drives.js +++ b/lib/gui/models/drives.js @@ -22,17 +22,12 @@ const angular = require('angular'); const _ = require('lodash'); +const store = require('./store'); const MODULE_NAME = 'Etcher.Models.Drives'; const Drives = angular.module(MODULE_NAME, []); Drives.service('DrivesModel', function() { - - /** - * @summary List of available drives - * @type {Object[]} - * @private - */ - let availableDrives = []; + const self = this; /** * @summary Check if there are available drives @@ -47,7 +42,7 @@ Drives.service('DrivesModel', function() { * } */ this.hasAvailableDrives = function() { - return !_.isEmpty(availableDrives); + return !_.isEmpty(self.getDrives()); }; /** @@ -64,22 +59,10 @@ Drives.service('DrivesModel', function() { * DrivesModel.setDrives([ ... ]); */ this.setDrives = function(drives) { - - if (!drives) { - throw new Error('Missing drives'); - } - - if (!_.isArray(drives) || !_.every(drives, _.isPlainObject)) { - throw new Error(`Invalid drives: ${drives}`); - } - - // Only update if something has changed - // to avoid unnecessary DOM manipulations - // angular.equals ignores $$hashKey by default - if (!angular.equals(availableDrives, drives)) { - availableDrives = drives; - } - + store.dispatch({ + type: 'SET_AVAILABLE_DRIVES', + data: drives + }); }; /** @@ -93,7 +76,7 @@ Drives.service('DrivesModel', function() { * const drives = DrivesModel.getDrives(); */ this.getDrives = function() { - return availableDrives; + return store.getState().toJS().availableDrives; }; }); diff --git a/lib/gui/models/selection-state.js b/lib/gui/models/selection-state.js index e39eda0b..5a02d999 100644 --- a/lib/gui/models/selection-state.js +++ b/lib/gui/models/selection-state.js @@ -22,19 +22,13 @@ const _ = require('lodash'); const angular = require('angular'); +const store = require('./store'); const MODULE_NAME = 'Etcher.Models.SelectionState'; const SelectionStateModel = angular.module(MODULE_NAME, []); SelectionStateModel.service('SelectionStateModel', function() { let self = this; - /** - * @summary Selection state - * @type Object - * @private - */ - let selection = {}; - /** * @summary Set a drive * @function @@ -42,14 +36,6 @@ SelectionStateModel.service('SelectionStateModel', function() { * * @param {Object} drive - drive * - * @throws Will throw if drive lacks `.device`. - * @throws Will throw if `drive.device` is not a string. - * @throws Will throw if drive lacks `.name`. - * @throws Will throw if `drive.name` is not a string. - * @throws Will throw if drive lacks `.size`. - * @throws Will throw if `drive.size` is not a number. - * @throws Will throw if there is an image and the drive is not large enough. - * * @example * SelectionStateModel.setDrive({ * device: '/dev/disk2', @@ -58,40 +44,10 @@ SelectionStateModel.service('SelectionStateModel', function() { * }); */ this.setDrive = function(drive) { - - if (!drive.device) { - throw new Error('Missing drive device'); - } - - if (!_.isString(drive.device)) { - throw new Error(`Invalid drive device: ${drive.device}`); - } - - if (!drive.name) { - throw new Error('Missing drive name'); - } - - if (!_.isString(drive.name)) { - throw new Error(`Invalid drive name: ${drive.name}`); - } - - if (!drive.size) { - throw new Error('Missing drive size'); - } - - if (!_.isNumber(drive.size)) { - throw new Error(`Invalid drive size: ${drive.size}`); - } - - if (!_.isBoolean(drive.protected)) { - throw new Error(`Invalid drive protected state: ${drive.protected}`); - } - - if (!self.isDriveLargeEnough(drive)) { - throw new Error('The drive is not large enough'); - } - - selection.drive = drive; + store.dispatch({ + type: 'SELECT_DRIVE', + data: drive + }); }; /** @@ -211,35 +167,16 @@ SelectionStateModel.service('SelectionStateModel', function() { * * @param {Object} image - image * - * @throws Will throw if image lacks `.path`. - * @throws Will throw if `image.path` is not a string. - * @throws Will throw if image lacks `.size`. - * @throws Will throw if `image.size` is not a number. - * * @example * SelectionStateModel.setImage({ * path: 'foo.img' * }); */ this.setImage = function(image) { - - if (!image.path) { - throw new Error('Missing image path'); - } - - if (!_.isString(image.path)) { - throw new Error(`Invalid image path: ${image.path}`); - } - - if (!image.size) { - throw new Error('Missing image size'); - } - - if (!_.isNumber(image.size)) { - throw new Error(`Invalid image size: ${image.size}`); - } - - selection.image = image; + store.dispatch({ + type: 'SELECT_IMAGE', + data: image + }); }; /** @@ -253,11 +190,7 @@ SelectionStateModel.service('SelectionStateModel', function() { * const drive = SelectionStateModel.getDrive(); */ this.getDrive = function() { - if (_.isEmpty(selection.drive)) { - return; - } - - return selection.drive; + return _.get(store.getState().toJS(), 'selection.drive'); }; /** @@ -271,7 +204,7 @@ SelectionStateModel.service('SelectionStateModel', function() { * const imagePath = SelectionStateModel.getImagePath(); */ this.getImagePath = function() { - return _.get(selection.image, 'path'); + return _.get(store.getState().toJS(), 'selection.image.path'); }; /** @@ -285,7 +218,7 @@ SelectionStateModel.service('SelectionStateModel', function() { * const imageSize = SelectionStateModel.getImageSize(); */ this.getImageSize = function() { - return _.get(selection.image, 'size'); + return _.get(store.getState().toJS(), 'selection.image.size'); }; /** @@ -328,7 +261,11 @@ SelectionStateModel.service('SelectionStateModel', function() { * @example * SelectionStateModel.removeDrive(); */ - this.removeDrive = _.partial(_.unset, selection, 'drive'); + this.removeDrive = function() { + store.dispatch({ + type: 'REMOVE_DRIVE' + }); + }; /** * @summary Remove image @@ -338,7 +275,11 @@ SelectionStateModel.service('SelectionStateModel', function() { * @example * SelectionStateModel.removeImage(); */ - this.removeImage = _.partial(_.unset, selection, 'image'); + this.removeImage = function() { + store.dispatch({ + type: 'REMOVE_IMAGE' + }); + }; /** * @summary Clear selections @@ -354,12 +295,16 @@ SelectionStateModel.service('SelectionStateModel', function() { * @example * SelectionStateModel.clear({ preserveImage: true }); */ - this.clear = function(options) { - if (options && options.preserveImage) { - selection = _.pick(selection, 'image'); - } else { - selection = {}; + this.clear = function(options = {}) { + if (!options.preserveImage) { + store.dispatch({ + type: 'REMOVE_IMAGE' + }); } + + store.dispatch({ + type: 'REMOVE_DRIVE' + }); }; /** diff --git a/lib/gui/models/store.js b/lib/gui/models/store.js new file mode 100644 index 00000000..7d539aec --- /dev/null +++ b/lib/gui/models/store.js @@ -0,0 +1,140 @@ +/* + * 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'; + +const Immutable = require('immutable'); +const _ = require('lodash'); +const redux = require('redux'); + +/** + * @summary Application default state + * @type Object + * @constant + * @private + */ +const DEFAULT_STATE = Immutable.fromJS({ + availableDrives: [], + selection: {}, + flash: { + flashing: false, + state: { + progress: 0, + speed: 0 + } + } +}); + +module.exports = redux.createStore(function(state, action) { + state = state || DEFAULT_STATE; + + switch (action.type) { + + case 'SET_AVAILABLE_DRIVES': { + if (!action.data) { + throw new Error('Missing drives'); + } + + if (!_.isArray(action.data) || !_.every(action.data, _.isPlainObject)) { + throw new Error(`Invalid drives: ${action.data}`); + } + + return state.set('availableDrives', Immutable.fromJS(action.data)); + } + + case 'SET_FLASH_STATE': { + return state.setIn([ 'flash', 'state' ], Immutable.fromJS(action.data)); + } + + case 'RESET_FLASH_STATE': { + return state.setIn([ 'flash', 'state' ], DEFAULT_STATE.getIn([ 'flash', 'state' ])); + } + + case 'SET_FLASHING': { + return state.setIn([ 'flash', 'flashing' ], Boolean(action.data)); + } + + case 'SELECT_DRIVE': { + if (!action.data.device) { + throw new Error('Missing drive device'); + } + + if (!_.isString(action.data.device)) { + throw new Error(`Invalid drive device: ${action.data.device}`); + } + + if (!action.data.name) { + throw new Error('Missing drive name'); + } + + if (!_.isString(action.data.name)) { + throw new Error(`Invalid drive name: ${action.data.name}`); + } + + if (!action.data.size) { + throw new Error('Missing drive size'); + } + + if (!_.isNumber(action.data.size)) { + throw new Error(`Invalid drive size: ${action.data.size}`); + } + + if (!_.isBoolean(action.data.protected)) { + throw new Error(`Invalid drive protected state: ${action.data.protected}`); + } + + // TODO: Reuse from SelectionStateModel.isDriveLargeEnough() + if (state.getIn([ 'selection', 'image', 'size' ], 0) > action.data.size) { + throw new Error('The drive is not large enough'); + } + + return state.setIn([ 'selection', 'drive' ], Immutable.fromJS(action.data)); + } + + case 'SELECT_IMAGE': { + if (!action.data.path) { + throw new Error('Missing image path'); + } + + if (!_.isString(action.data.path)) { + throw new Error(`Invalid image path: ${action.data.path}`); + } + + if (!action.data.size) { + throw new Error('Missing image size'); + } + + if (!_.isNumber(action.data.size)) { + throw new Error(`Invalid image size: ${action.data.size}`); + } + + return state.setIn([ 'selection', 'image' ], Immutable.fromJS(action.data)); + } + + case 'REMOVE_DRIVE': { + return state.deleteIn([ 'selection', 'drive' ]); + } + + case 'REMOVE_IMAGE': { + return state.deleteIn([ 'selection', 'image' ]); + } + + default: { + return state; + } + + } +}); diff --git a/lib/gui/modules/image-writer.js b/lib/gui/modules/image-writer.js index a3d83572..e4fac8ec 100644 --- a/lib/gui/modules/image-writer.js +++ b/lib/gui/modules/image-writer.js @@ -21,6 +21,7 @@ */ const angular = require('angular'); +const store = require('../models/store'); const childWriter = require('../../src/child-writer'); const MODULE_NAME = 'Etcher.image-writer'; @@ -31,7 +32,6 @@ const imageWriter = angular.module(MODULE_NAME, [ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel, NotifierService) { let self = this; - let flashing = false; /** * @summary Reset flash state @@ -42,10 +42,9 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel, * ImageWriterService.resetState(); */ this.resetState = function() { - self.state = { - progress: 0, - speed: 0 - }; + store.dispatch({ + type: 'RESET_FLASH_STATE' + }); }; /** @@ -53,8 +52,19 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel, * @type Object * @public */ - this.state = {}; - this.resetState(); + this.state = { + progress: 0, + speed: 0 + }; + + store.subscribe(function() { + + // Safely bring the state to the world of Angular + $timeout(function() { + self.state = store.getState().toJS().flash.state; + }); + + }); /** * @summary Check if currently flashing @@ -69,7 +79,7 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel, * } */ this.isFlashing = function() { - return flashing; + return store.getState().toJS().flash.flashing; }; /** @@ -86,7 +96,10 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel, * ImageWriterService.setFlashing(true); */ this.setFlashing = function(status) { - flashing = Boolean(status); + store.dispatch({ + type: 'SET_FLASHING', + data: status + }); }; /** @@ -150,21 +163,20 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel, return self.performWrite(image, drive, function(state) { - // Safely bring the state to the world of Angular - $timeout(function() { - - self.state = { + store.dispatch({ + type: 'SET_FLASH_STATE', + data: { type: state.type, progress: state.percentage, eta: state.eta, // Transform bytes to megabytes preserving only two decimal places speed: Math.floor(state.speed / 1e+6 * 100) / 100 || 0 - }; - - NotifierService.emit('image-writer:state', self.state); + } }); + NotifierService.emit('image-writer:state', self.state); + }).finally(function() { self.setFlashing(false); }); diff --git a/package.json b/package.json index 39a90d86..14e9a46a 100644 --- a/package.json +++ b/package.json @@ -69,9 +69,11 @@ "etcher-image-write": "^5.0.1", "file-tail": "^0.3.0", "flexboxgrid": "^6.3.0", + "immutable": "^3.8.1", "is-elevated": "^1.0.0", "lodash": "^4.5.1", "ngstorage": "^0.3.10", + "redux": "^3.5.2", "resin-cli-errors": "^1.2.0", "resin-cli-form": "^1.4.1", "resin-cli-visuals": "^1.2.8", diff --git a/tests/gui/models/selection-state.spec.js b/tests/gui/models/selection-state.spec.js index 13e92e14..fbc101f4 100644 --- a/tests/gui/models/selection-state.spec.js +++ b/tests/gui/models/selection-state.spec.js @@ -383,7 +383,7 @@ describe('Browser: SelectionState', function() { describe('.setDrive()', function() { - it('should throw if drive is no large enough', function() { + it('should throw if drive is not large enough', function() { m.chai.expect(function() { SelectionStateModel.setDrive({ device: '/dev/disk1', diff --git a/tests/gui/modules/image-writer.spec.js b/tests/gui/modules/image-writer.spec.js index 16d0de62..89fdba50 100644 --- a/tests/gui/modules/image-writer.spec.js +++ b/tests/gui/modules/image-writer.spec.js @@ -44,6 +44,7 @@ describe('Browser: ImageWriter', function() { }; ImageWriterService.resetState(); + $timeout.flush(); m.chai.expect(ImageWriterService.state).to.deep.equal({ progress: 0,