refactor(DriveScannerService): use RxJS (#505)

The concept of a drive scanner lends itself very well to the concept of
"reactive programming", since the "available drives" make perfect sense
as an "Observable".

For this reason, the module was re-implemented using RxJS, which greatly
simplifies things and erradicates certain edge cases we were protecting
ourselves from automatically.

Other changes made in this PR:

- `DriveScannerService` no longer requires `DrivesModel`. The
`DriveScannerService` is the one in charge of populating the model.

- `DriveScannerService.scan()` no longer returns an `EventEmitter`. The
service itself is now an `EventEmitter`.

- `DriveScannerService` now emits a `drives`, not a `scan` event.

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

lib/gui/app.js

@@ -155,7 +155,12 @@ app.controller('AppController', function(
     OSWindowProgressService.set(state.progress);
   });
 
-  DriveScannerService.start(2000).on('error', self.handleError).on('scan', function(drives) {
+  DriveScannerService.start();
+
+  DriveScannerService.on('error', self.handleError);
+
+  DriveScannerService.on('drives', function(drives) {
+    self.drives.setDrives(drives);
 
     // Cover the case where you select a drive, but then eject it.
     if (self.selection.hasDrive() && !_.find(drives, self.selection.isCurrentDrive)) {

lib/gui/modules/drive-scanner.js

@@ -17,90 +17,67 @@
 'use strict';
 
 /**
- * @module Etcher.drive-scanner
+ * @module Etcher.Modules.DriveScanner
  */
 
-const angular = require('angular');
+const Rx = require('rx');
 const _ = require('lodash');
+const angular = require('angular');
 const EventEmitter = require('events').EventEmitter;
 const drivelist = require('drivelist');
 
-const MODULE_NAME = 'Etcher.drive-scanner';
-const driveScanner = angular.module(MODULE_NAME, [
-  require('angular-q-promisify'),
-  require('../models/drives')
-]);
+const MODULE_NAME = 'Etcher.Modules.DriveScanner';
+const driveScanner = angular.module(MODULE_NAME, []);
 
-driveScanner.service('DriveScannerService', function($q, $interval, $timeout, DrivesModel) {
-  let self = this;
-  let interval = null;
+driveScanner.factory('DriveScannerService', function() {
+  const DRIVE_SCANNER_INTERVAL_MS = 2000;
+  const emitter = new EventEmitter();
+
+  const availableDrives = Rx.Observable.timer(0, DRIVE_SCANNER_INTERVAL_MS)
+    .flatMap(() => {
+      return Rx.Observable.fromNodeCallback(drivelist.list)();
+    })
+    .map((drives) => {
+      return _.reject(drives, (drive) => {
+        return drive.system;
+      });
+    })
+    .pausable(new Rx.Subject());
+
+  /*
+   * This service emits the following events:
+   *
+   * - `drives (Object[])`
+   * - `error (Error)`
+   *
+   * For example:
+   *
+   * ```
+   * DriveScannerService.on('drives', (drives) => {
+   *   console.log(drives);
+   * });
+   *
+   * DriveScannerService.on('error', (error) => {
+   *   throw error;
+   * });
+   * ```
+   */
+  availableDrives.subscribe((drives) => {
+    emitter.emit('drives', drives);
+  }, (error) => {
+    emitter.emit('error', error);
+  });
 
   /**
-   * @summary Get available drives
+   * @summary Start scanning drives
    * @function
    * @public
    *
-   * @fulfil {Object[]} - drives
-   * @returns {Promise}
-   *
    * @example
-   * DriveScannerService.scan().then(function(drives) {
-   *   console.log(drives);
-   * });
+   * DriveScannerService.start();
    */
-  this.scan = function() {
-    return $q.promisify(drivelist.list)().then(function(drives) {
-      return _.filter(drives, function(drive) {
-        return !drive.system;
-      });
-    });
-  };
-
-  /**
-   * @summary Scan drives and populate DrivesModel
-   * @function
-   * @public
-   *
-   * @description
-   * This function returns an event emitter instance
-   * that emits a `scan` event everything it scans
-   * the drives successfully.
-   *
-   * @param {Number} ms - interval milliseconds
-   * @returns {EventEmitter} event emitter instance
-   *
-   * @example
-   * const emitter = DriveScannerService.start(2000);
-   *
-   * emitter.on('scan', function(drives) {
-   *   console.log(drives);
-   * });
-   */
-  this.start = function(ms) {
-    let emitter = new EventEmitter();
-
-    const fn = function() {
-      return self.scan().then(function(drives) {
-        emitter.emit('scan', drives);
-        DrivesModel.setDrives(drives);
-      }).catch(function(error) {
-        emitter.emit('error', error);
-      });
-    };
-
-    // Make sure any pending interval is cancelled
-    // to avoid potential memory leaks.
-    self.stop();
-
-    // Call fn after in the next process tick
-    // to be able to capture the first run
-    // in unit tests.
-    $timeout(function() {
-      fn();
-      interval = $interval(fn, ms);
-    });
-
-    return emitter;
+  emitter.start = () => {
+    availableDrives.resume();
   };
 
   /**
@@ -111,10 +88,11 @@ driveScanner.service('DriveScannerService', function($q, $interval, $timeout, Dr
    * @example
    * DriveScannerService.stop();
    */
-  this.stop = function() {
-    $interval.cancel(interval);
+  emitter.stop = () => {
+    availableDrives.pause();
   };
 
+  return emitter;
 });
 
 module.exports = MODULE_NAME;

npm-shrinkwrap.json

@@ -64,11 +64,6 @@
       "from": "angular-moment@>=1.0.0-beta.6 <2.0.0",
       "resolved": "https://registry.npmjs.org/angular-moment/-/angular-moment-1.0.0-beta.6.tgz"
     },
-    "angular-q-promisify": {
-      "version": "1.1.0",
-      "from": "angular-q-promisify@>=1.1.0 <2.0.0",
-      "resolved": "https://registry.npmjs.org/angular-q-promisify/-/angular-q-promisify-1.1.0.tgz"
-    },
     "angular-ui-bootstrap": {
       "version": "1.3.3",
       "from": "angular-ui-bootstrap@>=1.3.2 <2.0.0",
@@ -4678,6 +4673,11 @@
       "from": "run-series@>=1.1.4 <2.0.0",
       "resolved": "https://registry.npmjs.org/run-series/-/run-series-1.1.4.tgz"
     },
+    "rx": {
+      "version": "4.1.0",
+      "from": "rx@latest",
+      "resolved": "https://registry.npmjs.org/rx/-/rx-4.1.0.tgz"
+    },
     "rx-lite": {
       "version": "3.1.2",
       "from": "rx-lite@>=3.1.2 <4.0.0",

package.json

@@ -58,7 +58,6 @@
     "angular-if-state": "^1.0.0",
     "angular-middle-ellipses": "^1.0.0",
     "angular-moment": "^1.0.0-beta.6",
-    "angular-q-promisify": "^1.1.0",
     "angular-ui-bootstrap": "^1.3.2",
     "angular-ui-router": "^0.2.18",
     "bluebird": "^3.0.5",
@@ -76,6 +75,7 @@
     "resin-cli-errors": "^1.2.0",
     "resin-cli-form": "^1.4.1",
     "resin-cli-visuals": "^1.2.8",
+    "rx": "^4.1.0",
     "semver": "^5.1.0",
     "sudo-prompt": "^5.0.3",
     "tail": "^1.1.0",

tests/gui/modules/drive-scanner.spec.js

@@ -11,166 +11,64 @@ describe('Browser: DriveScanner', function() {
     require('../../../lib/gui/modules/drive-scanner')
   ));
 
-  beforeEach(angular.mock.module(
-    require('../../../lib/gui/models/drives')
-  ));
-
   describe('DriveScannerService', function() {
 
-    let $interval;
-    let $rootScope;
-    let $timeout;
-    let $q;
-    let DrivesModel;
     let DriveScannerService;
 
-    beforeEach(angular.mock.inject(function(_$interval_, _$rootScope_, _$timeout_, _$q_, _DriveScannerService_, _DrivesModel_) {
-      $interval = _$interval_;
-      $rootScope = _$rootScope_;
-      $timeout = _$timeout_;
-      $q = _$q_;
+    beforeEach(angular.mock.inject(function(_DriveScannerService_) {
       DriveScannerService = _DriveScannerService_;
-      DrivesModel = _DrivesModel_;
     }));
 
-    describe('.scan()', function() {
-
-      describe('given no available drives', function() {
-
-        beforeEach(function() {
-          this.drivesListStub = m.sinon.stub(drivelist, 'list');
-          this.drivesListStub.yields(null, []);
-        });
-
-        afterEach(function() {
-          this.drivesListStub.restore();
-        });
-
-        it('should eventually equal an empty array', function(done) {
-          DriveScannerService.scan().then(function(drives) {
-            m.chai.expect(drives).to.deep.equal([]);
-            done();
-          });
-
-          $rootScope.$apply();
-        });
+    describe('given no available drives', function() {
 
+      beforeEach(function() {
+        this.drivesListStub = m.sinon.stub(drivelist, 'list');
+        this.drivesListStub.yields(null, []);
       });
 
-      describe('given available system drives', function() {
-
-        beforeEach(function() {
-          this.drives = [
-            {
-              device: '/dev/sda',
-              description: 'WDC WD10JPVX-75J',
-              size: '931.5G',
-              mountpoint: '/',
-              system: true
-            }
-          ];
-
-          this.drivesListStub = m.sinon.stub(drivelist, 'list');
-          this.drivesListStub.yields(null, this.drives);
-        });
-
-        afterEach(function() {
-          this.drivesListStub.restore();
-        });
-
-        it('should eventually equal an empty array', function(done) {
-          DriveScannerService.scan().then(function(drives) {
-            m.chai.expect(drives).to.deep.equal([]);
-            done();
-          });
-
-          $rootScope.$apply();
-        });
-
+      afterEach(function() {
+        this.drivesListStub.restore();
       });
 
-      describe('given available system and removable drives', function() {
-
-        beforeEach(function() {
-          this.drives = [
-            {
-              device: '/dev/sda',
-              description: 'WDC WD10JPVX-75J',
-              size: '931.5G',
-              mountpoint: '/',
-              system: true
-            },
-            {
-              device: '/dev/sdb',
-              description: 'Foo',
-              size: '14G',
-              mountpoint: '/mnt/foo',
-              system: false
-            },
-            {
-              device: '/dev/sdc',
-              description: 'Bar',
-              size: '14G',
-              mountpoint: '/mnt/bar',
-              system: false
-            }
-          ];
-
-          this.drivesListStub = m.sinon.stub(drivelist, 'list');
-          this.drivesListStub.yields(null, this.drives);
-        });
-
-        afterEach(function() {
-          this.drivesListStub.restore();
-        });
-
-        it('should eventually become the removable drives', function(done) {
-          DriveScannerService.scan().then(function(drives) {
-            m.chai.expect(drives).to.deep.equal([
-              {
-                device: '/dev/sdb',
-                description: 'Foo',
-                size: '14G',
-                mountpoint: '/mnt/foo',
-                system: false
-              },
-              {
-                device: '/dev/sdc',
-                description: 'Bar',
-                size: '14G',
-                mountpoint: '/mnt/bar',
-                system: false
-              }
-            ]);
-            done();
-          });
-
-          $rootScope.$apply();
+      it('should emit an empty array', function(done) {
+        DriveScannerService.on('drives', function(drives) {
+          m.chai.expect(drives).to.deep.equal([]);
+          DriveScannerService.stop();
+          done();
         });
 
+        DriveScannerService.start();
       });
 
-      describe('given an error when listing the drives', function() {
+    });
 
-        beforeEach(function() {
-          this.drivesListStub = m.sinon.stub(drivelist, 'list');
-          this.drivesListStub.yields(new Error('scan error'));
-        });
-
-        afterEach(function() {
-          this.drivesListStub.restore();
-        });
-
-        it('should be rejected with the error', function(done) {
-          DriveScannerService.scan().catch(function(error) {
-            m.chai.expect(error).to.be.an.instanceof(Error);
-            m.chai.expect(error.message).to.equal('scan error');
-            done();
-          });
-
-          $rootScope.$apply();
+    describe('given only system available drives', function() {
+
+      beforeEach(function() {
+        this.drivesListStub = m.sinon.stub(drivelist, 'list');
+        this.drivesListStub.yields(null, [
+          {
+            device: '/dev/sda',
+            description: 'WDC WD10JPVX-75J',
+            size: '931.5G',
+            mountpoint: '/',
+            system: true
+          }
+        ]);
+      });
+
+      afterEach(function() {
+        this.drivesListStub.restore();
+      });
+
+      it('should emit an empty array', function(done) {
+        DriveScannerService.on('drives', function(drives) {
+          m.chai.expect(drives).to.deep.equal([]);
+          DriveScannerService.stop();
+          done();
         });
 
+        DriveScannerService.start();
       });
 
     });
@@ -178,7 +76,15 @@ describe('Browser: DriveScanner', function() {
     describe('given available drives', function() {
 
       beforeEach(function() {
-        this.drives = [
+        this.drivesListStub = m.sinon.stub(drivelist, 'list');
+        this.drivesListStub.yields(null, [
+          {
+            device: '/dev/sda',
+            description: 'WDC WD10JPVX-75J',
+            size: '931.5G',
+            mountpoint: '/',
+            system: true
+          },
           {
             device: '/dev/sdb',
             description: 'Foo',
@@ -193,37 +99,61 @@ describe('Browser: DriveScanner', function() {
             mountpoint: '/mnt/bar',
             system: false
           }
-        ];
-
-        this.scanStub = m.sinon.stub(DriveScannerService, 'scan');
-        this.scanStub.returns($q.resolve(this.drives));
+        ]);
       });
 
       afterEach(function() {
-        this.scanStub.restore();
+        this.drivesListStub.restore();
       });
 
-      it('should set the drives to the scanned ones', function() {
-        DriveScannerService.start(200);
-        $timeout.flush();
-        $interval.flush(400);
-        m.chai.expect(DrivesModel.getDrives()).to.deep.equal(this.drives);
-        DriveScannerService.stop();
-      });
+      it('should emit the non removable drives', function(done) {
+        DriveScannerService.on('drives', function(drives) {
+          m.chai.expect(drives).to.deep.equal([
+            {
+              device: '/dev/sdb',
+              description: 'Foo',
+              size: '14G',
+              mountpoint: '/mnt/foo',
+              system: false
+            },
+            {
+              device: '/dev/sdc',
+              description: 'Bar',
+              size: '14G',
+              mountpoint: '/mnt/bar',
+              system: false
+            }
+          ]);
 
-      describe('.start()', function() {
-
-        it('should emit a `scan` event with the drives', function() {
-          const emitter = DriveScannerService.start(2000);
-          const scanSpy = m.sinon.spy();
-          emitter.on('scan', scanSpy);
-          $timeout.flush();
-          $interval.flush(1000);
-          m.chai.expect(scanSpy).to.have.been.calledOnce;
-          m.chai.expect(scanSpy).to.have.been.calledWith(this.drives);
           DriveScannerService.stop();
+          done();
         });
 
+        DriveScannerService.start();
+      });
+
+    });
+
+    describe('given an error when listing the drives', function() {
+
+      beforeEach(function() {
+        this.drivesListStub = m.sinon.stub(drivelist, 'list');
+        this.drivesListStub.yields(new Error('scan error'));
+      });
+
+      afterEach(function() {
+        this.drivesListStub.restore();
+      });
+
+      it('should emit the error', function(done) {
+        DriveScannerService.on('error', function(error) {
+          m.chai.expect(error).to.be.an.instanceof(Error);
+          m.chai.expect(error.message).to.equal('scan error');
+          DriveScannerService.stop();
+          done();
+        });
+
+        DriveScannerService.start();
       });
 
     });