refactor(GUI): decouple localStorage from the redux store (#1569)

We're currently persisting the user settings in localSettings by using a
redux plugin called redux-localstorage. As a way to decouple the redux
store from a technology that is browser specific, this commit makes the
following changes:

- Create local-settings.js, which is concerned with managing settings in
  a persisting location

- Decouple the redux store from the persisting storage method

- Extend the settings model to persist settings, cache reads, etc

Change-Type: patch
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

dictionary

@@ -2,3 +2,4 @@ boolen->boolean
 aknowledge->acknowledge
 seleted->selected
 reming->remind
+locl->local

lib/gui/app.js

@@ -88,6 +88,7 @@ app.run(() => {
 
 app.run((ErrorService) => {
   analytics.logEvent('Application start');
+  settings.load();
 
   const currentVersion = packageJSON.version;
   const shouldCheckForUpdates = updateNotifier.shouldCheckForUpdates({

lib/gui/models/local-settings.js

@@ -0,0 +1,70 @@
+/*
+ * Copyright 2017 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';
+
+/**
+ * @summary Local storage settings key
+ * @constant
+ * @type {String}
+ */
+const LOCAL_STORAGE_SETTINGS_KEY = 'etcher-settings';
+
+/**
+ * @summary Read all local settings
+ * @function
+ * @public
+ *
+ * @returns {Object} local settings
+ *
+ * @example
+ * const settings = localSettings.readAll();
+ */
+exports.readAll = () => {
+  return JSON.parse(localStorage.getItem(LOCAL_STORAGE_SETTINGS_KEY)) || {};
+};
+
+/**
+ * @summary Write local settings
+ * @function
+ * @public
+ *
+ * @param {Object} settings - settings
+ *
+ * @example
+ * localSettings.writeAll({
+ *   foo: 'bar'
+ * });
+ */
+exports.writeAll = (settings) => {
+  const INDENTATION_SPACES = 2;
+  localStorage.setItem(LOCAL_STORAGE_SETTINGS_KEY, JSON.stringify(settings, null, INDENTATION_SPACES));
+};
+
+/**
+ * @summary Clear the local settings
+ * @function
+ * @private
+ *
+ * @description
+ * Exported for testing purposes
+ *
+ * @example
+ * localSettings.clear();
+ */
+exports.clear = () => {
+  localStorage.removeItem(LOCAL_STORAGE_SETTINGS_KEY);
+};

lib/gui/models/settings.js

@@ -22,8 +22,100 @@
 
 const _ = require('lodash');
 const Store = require('./store');
+const localSettings = require('./local-settings');
 const errors = require('../../shared/errors');
 
+/**
+ * @summary Set a settings object
+ * @function
+ * @private
+ *
+ * @description
+ * Use this function with care, given that it will completely
+ * override any existing settings in both the redux store,
+ * and the local user configuration.
+ *
+ * This function is prepared to deal with any local configuration
+ * write issues by rolling back to the previous settings if so.
+ *
+ * @param {Object} settings - settings
+ *
+ * @example
+ * setSettingsObject({ foo: 'bar' });
+ */
+const setSettingsObject = (settings) => {
+  const currentSettings = exports.getAll();
+
+  Store.dispatch({
+    type: Store.Actions.SET_SETTINGS,
+    data: settings
+  });
+
+  const result = _.attempt(localSettings.writeAll, settings);
+
+  // Revert the application state if writing the data
+  // to the local machine was not successful
+  if (_.isError(result)) {
+    Store.dispatch({
+      type: Store.Actions.SET_SETTINGS,
+      data: currentSettings
+    });
+
+    throw result;
+  }
+};
+
+/**
+ * @summary Default settings
+ * @constant
+ * @type {Object}
+ */
+const DEFAULT_SETTINGS = Store.Defaults.get('settings').toJS();
+
+/**
+ * @summary Reset settings to their default values
+ * @function
+ * @public
+ *
+ * @example
+ * settings.reset();
+ */
+exports.reset = _.partial(setSettingsObject, DEFAULT_SETTINGS);
+
+/**
+ * @summary Extend the current settings
+ * @function
+ * @public
+ *
+ * @param {Object} settings - settings
+ *
+ * @example
+ * settings.assign({
+ *   foo: 'bar'
+ * });
+ */
+exports.assign = (settings) => {
+  if (_.isNil(settings)) {
+    throw errors.createError({
+      title: 'Missing settings'
+    });
+  }
+
+  setSettingsObject(_.assign(exports.getAll(), settings));
+};
+
+/**
+ * @summary Extend the application state with the local settings
+ * @function
+ * @public
+ *
+ * @example
+ * settings.load();
+ */
+exports.load = () => {
+  exports.assign(localSettings.readAll());
+};
+
 /**
  * @summary Set a setting value
  * @function
@@ -48,14 +140,9 @@ exports.set = (key, value) => {
     });
   }
 
-  const newSettings = _.assign(exports.getAll(), {
+  exports.assign({
     [key]: value
   });
-
-  Store.dispatch({
-    type: Store.Actions.SET_SETTINGS,
-    data: newSettings
-  });
 };
 
 /**
@@ -70,7 +157,7 @@ exports.set = (key, value) => {
  * const value = settings.get('unmountOnSuccess');
  */
 exports.get = (key) => {
-  return this.getAll()[key];
+  return _.get(exports.getAll(), [ key ]);
 };
 
 /**

lib/gui/models/store.js

@@ -19,7 +19,6 @@
 const Immutable = require('immutable');
 const _ = require('lodash');
 const redux = require('redux');
-const persistState = require('redux-localstorage');
 const uuidV4 = require('uuid/v4');
 const constraints = require('../../shared/drive-constraints');
 const supportedFormats = require('../../shared/supported-formats');
@@ -56,14 +55,6 @@ const DEFAULT_STATE = Immutable.fromJS({
   }
 });
 
-/**
- * @summary State path to be persisted
- * @type {Object}
- * @constant
- * @private
- */
-const PERSISTED_PATH = 'settings';
-
 /**
  * @summary Application supported action messages
  * @type {Object}
@@ -495,56 +486,7 @@ const storeReducer = (state = DEFAULT_STATE, action) => {
   }
 };
 
-module.exports = _.merge(redux.createStore(
-  storeReducer,
-  DEFAULT_STATE,
-  redux.compose(persistState(PERSISTED_PATH, {
-
-    // The following options are set for the sole
-    // purpose of dealing correctly with ImmutableJS
-    // collections.
-    // See: https://github.com/elgerlambert/redux-localstorage#immutable-data
-
-    slicer: (key) => {
-      return (state) => {
-        return state.get(key);
-      };
-    },
-
-    serialize: (collection) => {
-      return JSON.stringify(collection.toJS());
-    },
-
-    deserialize: (data) => {
-      return Immutable.fromJS(JSON.parse(data));
-    },
-
-    merge: (state, subset) => {
-
-      // In the first run, there will be no information
-      // to deserialize. In this case, we avoid merging,
-      // otherwise we will be basically erasing the property
-      // we aim to keep serialising the in future.
-      if (!subset) {
-        return state;
-      }
-
-      // Blindly setting the state to the deserialised subset
-      // means that a user could manually edit `localStorage`
-      // and extend the application state settings with
-      // unsupported properties, since it can bypass validation.
-      //
-      // The alternative, which would be dispatching each
-      // deserialised settins through the appropriate action
-      // is not very elegant, nor performant, so we decide
-      // to intentionally ignore this little flaw since
-      // adding extra properties makes no damage at all.
-      return state.set(PERSISTED_PATH, state.get(PERSISTED_PATH).merge(subset));
-
-    }
-
-  }))
-), {
+module.exports = _.merge(redux.createStore(storeReducer, DEFAULT_STATE), {
   Actions: ACTIONS,
   Defaults: DEFAULT_STATE
 });

npm-shrinkwrap.json

@@ -8955,11 +8955,6 @@
       "from": "redux@3.5.2",
       "resolved": "https://registry.npmjs.org/redux/-/redux-3.5.2.tgz"
     },
-    "redux-localstorage": {
-      "version": "0.4.1",
-      "from": "redux-localstorage@0.4.1",
-      "resolved": "https://registry.npmjs.org/redux-localstorage/-/redux-localstorage-0.4.1.tgz"
-    },
     "regenerator": {
       "version": "0.8.46",
       "from": "regenerator@>=0.8.13 <0.9.0",

package.json

@@ -65,7 +65,6 @@
     "react-dom": "15.5.4",
     "react2angular": "1.1.3",
     "redux": "3.5.2",
-    "redux-localstorage": "0.4.1",
     "request": "2.81.0",
     "resin-cli-form": "1.4.1",
     "resin-cli-visuals": "1.3.1",

tests/gui/models/settings.spec.js

@@ -4,31 +4,150 @@ const m = require('mochainon');
 const _ = require('lodash');
 const Store = require('../../../lib/gui/models/store');
 const settings = require('../../../lib/gui/models/settings');
+const localSettings = require('../../../lib/gui/models/local-settings');
 
 describe('Browser: settings', function() {
 
-  describe('settings', function() {
-
-    const DEFAULT_KEYS = _.keys(Store.Defaults.get('settings').toJS());
-
   beforeEach(function() {
-      this.settings = settings.getAll();
+    settings.reset();
   });
 
-    afterEach(function() {
-      _.each(DEFAULT_KEYS, (supportedKey) => {
-        settings.set(supportedKey, this.settings[supportedKey]);
-      });
-    });
+  const DEFAULT_SETTINGS = Store.Defaults.get('settings').toJS();
 
   it('should be able to set and read values', function() {
-      const keyUnderTest = _.sample(DEFAULT_KEYS);
-      const originalValue = settings.get(keyUnderTest);
+    m.chai.expect(settings.get('foo')).to.be.undefined;
+    settings.set('foo', true);
+    m.chai.expect(settings.get('foo')).to.be.true;
+    settings.set('foo', false);
+    m.chai.expect(settings.get('foo')).to.be.false;
+  });
+
+  describe('.reset()', function() {
+
+    it('should reset the settings to their default values', function() {
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
+      settings.set('foo', 1234);
+      m.chai.expect(settings.getAll()).to.not.deep.equal(DEFAULT_SETTINGS);
+      settings.reset();
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
+    });
+
+    it('should reset the local settings to their default values', function() {
+      settings.set('foo', 1234);
+      m.chai.expect(localSettings.readAll()).to.not.deep.equal(DEFAULT_SETTINGS);
+      settings.reset();
+      m.chai.expect(localSettings.readAll()).to.deep.equal(DEFAULT_SETTINGS);
+    });
+
+    describe('given the local settings are cleared', function() {
+
+      beforeEach(function() {
+        localSettings.clear();
+      });
+
+      it('should set the local settings to their default values', function() {
+        settings.reset();
+        m.chai.expect(localSettings.readAll()).to.deep.equal(DEFAULT_SETTINGS);
+      });
+
+    });
+
+  });
+
+  describe('.assign()', function() {
+
+    it('should throw if no settings', function() {
+      m.chai.expect(function() {
+        settings.assign();
+      }).to.throw('Missing setting');
+    });
+
+    it('should throw if setting an array', function() {
+      m.chai.expect(function() {
+        settings.assign({
+          foo: 'bar',
+          bar: [ 1, 2, 3 ]
+        });
+      }).to.throw('Invalid setting value: 1,2,3 for bar');
+    });
+
+    it('should not override all settings', function() {
+      settings.assign({
+        foo: 'bar',
+        bar: 'baz'
+      });
+
+      m.chai.expect(settings.getAll()).to.deep.equal(_.assign({}, DEFAULT_SETTINGS, {
+        foo: 'bar',
+        bar: 'baz'
+      }));
+    });
+
+    it('should not store invalid settings to the local machine', function() {
+      m.chai.expect(localSettings.readAll().foo).to.be.undefined;
+
+      m.chai.expect(() => {
+        settings.assign({
+          foo: [ 1, 2, 3 ]
+        });
+      }).to.throw('Invalid setting value: 1,2,3');
+
+      m.chai.expect(localSettings.readAll().foo).to.be.undefined;
+    });
+
+    it('should store the settings to the local machine', function() {
+      m.chai.expect(localSettings.readAll().foo).to.be.undefined;
+      m.chai.expect(localSettings.readAll().bar).to.be.undefined;
+
+      settings.assign({
+        foo: 'bar',
+        bar: 'baz'
+      });
+
+      m.chai.expect(localSettings.readAll().foo).to.equal('bar');
+      m.chai.expect(localSettings.readAll().bar).to.equal('baz');
+    });
+
+    it('should not change the application state if storing to the local machine results in an error', function() {
+      settings.set('foo', 'bar');
+      m.chai.expect(settings.get('foo')).to.equal('bar');
+
+      const localSettingsWriteAllStub = m.sinon.stub(localSettings, 'writeAll');
+      localSettingsWriteAllStub.throws(new Error('localSettings error'));
+
+      m.chai.expect(() => {
+        settings.assign({
+          foo: 'baz'
+        });
+      }).to.throw('localSettings error');
+
+      localSettingsWriteAllStub.restore();
+      m.chai.expect(settings.get('foo')).to.equal('bar');
+    });
+
+  });
+
+  describe('.load()', function() {
+
+    it('should extend the application state with the local settings content', function() {
+      const object = {
+        foo: 'bar'
+      };
+
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
+      localSettings.writeAll(object);
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
+      settings.load();
+      m.chai.expect(settings.getAll()).to.deep.equal(_.assign({}, DEFAULT_SETTINGS, object));
+    });
+
+    it('should keep the application state intact if there are no local settings', function() {
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
+      localSettings.clear();
+      settings.load();
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
+    });
 
-      settings.set(keyUnderTest, !originalValue);
-      m.chai.expect(settings.get(keyUnderTest)).to.equal(!originalValue);
-      settings.set(keyUnderTest, originalValue);
-      m.chai.expect(settings.get(keyUnderTest)).to.equal(originalValue);
   });
 
   describe('.set()', function() {
@@ -52,40 +171,65 @@ describe('Browser: settings', function() {
     });
 
     it('should throw if setting an object', function() {
-        const keyUnderTest = _.sample(DEFAULT_KEYS);
       m.chai.expect(function() {
-          settings.set(keyUnderTest, {
+        settings.set('foo', {
           setting: 1
         });
-        }).to.throw(`Invalid setting value: [object Object] for ${keyUnderTest}`);
+      }).to.throw('Invalid setting value: [object Object] for foo');
     });
 
     it('should throw if setting an array', function() {
-        const keyUnderTest = _.sample(DEFAULT_KEYS);
       m.chai.expect(function() {
-          settings.set(keyUnderTest, [ 1, 2, 3 ]);
-        }).to.throw(`Invalid setting value: 1,2,3 for ${keyUnderTest}`);
+        settings.set('foo', [ 1, 2, 3 ]);
+      }).to.throw('Invalid setting value: 1,2,3 for foo');
     });
 
     it('should set the key to undefined if no value', function() {
-        const keyUnderTest = _.sample(DEFAULT_KEYS);
-        settings.set(keyUnderTest);
-        m.chai.expect(settings.get(keyUnderTest)).to.be.undefined;
+      settings.set('foo', 'bar');
+      m.chai.expect(settings.get('foo')).to.equal('bar');
+      settings.set('foo');
+      m.chai.expect(settings.get('foo')).to.be.undefined;
+    });
+
+    it('should store the setting to the local machine', function() {
+      m.chai.expect(localSettings.readAll().foo).to.be.undefined;
+      settings.set('foo', 'bar');
+      m.chai.expect(localSettings.readAll().foo).to.equal('bar');
+    });
+
+    it('should not store invalid settings to the local machine', function() {
+      m.chai.expect(localSettings.readAll().foo).to.be.undefined;
+
+      m.chai.expect(() => {
+        settings.set('foo', [ 1, 2, 3 ]);
+      }).to.throw('Invalid setting value: 1,2,3');
+
+      m.chai.expect(localSettings.readAll().foo).to.be.undefined;
+    });
+
+    it('should not change the application state if storing to the local machine results in an error', function() {
+      settings.set('foo', 'bar');
+      m.chai.expect(settings.get('foo')).to.equal('bar');
+
+      const localSettingsWriteAllStub = m.sinon.stub(localSettings, 'writeAll');
+      localSettingsWriteAllStub.throws(new Error('localSettings error'));
+
+      m.chai.expect(() => {
+        settings.set('foo', 'baz');
+      }).to.throw('localSettings error');
+
+      localSettingsWriteAllStub.restore();
+      m.chai.expect(settings.get('foo')).to.equal('bar');
     });
 
   });
 
   describe('.getAll()', function() {
 
-      it('should be able to read all values', function() {
-        const allValues = settings.getAll();
-
-        _.each(DEFAULT_KEYS, function(supportedKey) {
-          m.chai.expect(allValues[supportedKey]).to.equal(settings.get(supportedKey));
-        });
+    it('should initial return all default values', function() {
+      m.chai.expect(settings.getAll()).to.deep.equal(DEFAULT_SETTINGS);
     });
 
   });
 
-  });
 });