Removes event name in unsubscribe API + Tests (#2364) (#2844)

js/src/api/api.js

@@ -90,8 +90,8 @@ export default class Api {
     return this._subscriptions.subscribe(subscriptionName, callback);
   }
 
-  unsubscribe (subscriptionName, subscriptionId) {
+  unsubscribe (subscriptionId) {
-    return this._subscriptions.unsubscribe(subscriptionName, subscriptionId);
+    return this._subscriptions.unsubscribe(subscriptionId);
   }
 
   pollMethod (method, input, validate) {

js/src/api/subscriptions/manager.js

@@ -29,17 +29,14 @@ const events = {
   'personal_requestsToConfirm': { module: 'signer' }
 };
 
-let nextSubscriptionId = 0;
-
 export default class Manager {
   constructor (api) {
     this._api = api;
 
-    this.subscriptions = {};
+    this.subscriptions = [];
     this.values = {};
 
     Object.keys(events).forEach((subscriptionName) => {
-      this.subscriptions[subscriptionName] = {};
       this.values[subscriptionName] = {
         error: null,
         data: null
@@ -71,60 +68,58 @@ export default class Manager {
         return;
       }
 
-      const subscriptionId = nextSubscriptionId++;
+      const subscriptionId = this.subscriptions.length;
       const { error, data } = this.values[subscriptionName];
       const engine = this[`_${subscription.module}`];
 
-      this.subscriptions[subscriptionName][subscriptionId] = callback;
+      this.subscriptions[subscriptionId] = {
+        name: subscriptionName,
+        id: subscriptionId,
+        callback
+      };
 
       if (!engine.isStarted) {
         engine.start();
       } else {
-        this._sendData(subscriptionName, subscriptionId, callback, error, data);
+        this._sendData(subscriptionId, error, data);
       }
 
       resolve(subscriptionId);
     });
   }
 
-  unsubscribe (subscriptionName, subscriptionId) {
+  unsubscribe (subscriptionId) {
     return new Promise((resolve, reject) => {
-      const subscription = this._validateType(subscriptionName);
+      if (!this.subscriptions[subscriptionId]) {
-
+        reject(new Error(`Cannot find subscription ${subscriptionId}`));
-      if (isError(subscription)) {
-        reject(subscription);
         return;
       }
 
-      if (!this.subscriptions[subscriptionName][subscriptionId]) {
+      delete this.subscriptions[subscriptionId];
-        reject(new Error(`Cannot find subscription ${subscriptionId} for type ${subscriptionName}`));
-        return;
-      }
-
-      delete this.subscriptions[subscriptionName][subscriptionId];
       resolve();
     });
   }
 
-  _sendData (subscriptionName, subscriptionId, callback, error, data) {
+  _sendData (subscriptionId, error, data) {
+    const { callback } = this.subscriptions[subscriptionId];
+
     try {
       callback(error, data);
     } catch (error) {
-      console.error(`Unable to update callback for ${subscriptionName}, subscriptionId ${subscriptionId}`, error);
+      console.error(`Unable to update callback for subscriptionId ${subscriptionId}`, error);
-      this.unsubscribe(subscriptionName, subscriptionId);
+      this.unsubscribe(subscriptionId);
     }
   }
 
   _updateSubscriptions = (subscriptionName, error, data) => {
-    if (!this.subscriptions[subscriptionName]) {
+    const subscriptions = this.subscriptions
-      throw new Error(`Cannot find entry point for subscriptions of type ${subscriptionName}`);
+      .filter(subscription => subscription.name === subscriptionName);
-    }
 
     this.values[subscriptionName] = { error, data };
-    Object.keys(this.subscriptions[subscriptionName]).forEach((subscriptionId) => {
-      const callback = this.subscriptions[subscriptionName][subscriptionId];
 
-      this._sendData(subscriptionName, subscriptionId, callback, error, data);
+    subscriptions
+      .forEach((subscription) => {
+        this._sendData(subscription.id, error, data);
       });
   }
 }

js/src/api/subscriptions/manager.spec.js

@@ -20,6 +20,7 @@ import Manager, { events } from './manager';
 
 function newStub () {
   const start = () => manager._updateSubscriptions(manager.__test, null, 'test');
+
   const manager = new Manager({
     transport: {
       isConnected: true
@@ -53,7 +54,7 @@ describe('api/subscriptions/manager', () => {
 
   describe('constructor', () => {
     it('sets up the subscription types & defaults', () => {
-      expect(Object.keys(manager.subscriptions)).to.deep.equal(Object.keys(events));
+      expect(manager.subscriptions).to.be.an.array;
       expect(Object.keys(manager.values)).to.deep.equal(Object.keys(events));
     });
   });
@@ -74,6 +75,7 @@ describe('api/subscriptions/manager', () => {
             manager.__test = eventName;
             cb = sinon.stub();
             sinon.spy(engine, 'start');
+
             return manager
               .subscribe(eventName, cb)
               .then((_subscriptionId) => {
@@ -86,7 +88,7 @@ describe('api/subscriptions/manager', () => {
           });
 
           it('returns a subscriptionId', () => {
-            expect(subscriptionId).to.be.ok;
+            expect(subscriptionId).to.be.a.number;
           });
 
           it('calls the subscription callback with updated values', () => {
@@ -95,4 +97,38 @@ describe('api/subscriptions/manager', () => {
         });
       });
   });
+
+  describe('unsubscriptions', () => {
+    Object
+      .keys(events)
+      .filter((eventName) => eventName.indexOf('_') !== -1)
+      .forEach((eventName) => {
+        const { module } = events[eventName];
+        let engine;
+        let cb;
+
+        describe(eventName, () => {
+          beforeEach(() => {
+            engine = manager[`_${module}`];
+            manager.__test = eventName;
+            cb = sinon.stub();
+            sinon.spy(engine, 'start');
+
+            return manager
+              .subscribe(eventName, cb)
+              .then((_subscriptionId) => {
+                manager.unsubscribe(_subscriptionId);
+              })
+              .then(() => {
+                manager._updateSubscriptions(manager.__test, null, 'test2');
+              });
+          });
+
+          it('does not call the callback after unsibscription', () => {
+            expect(cb).to.have.been.calledWith(null, 'test');
+            expect(cb).to.not.have.been.calledWith(null, 'test2');
+          });
+        });
+      });
+  });
 });

js/src/ui/TxHash/txHash.js

@@ -50,7 +50,7 @@ class TxHash extends Component {
     const { api } = this.context;
     const { subscriptionId } = this.state;
 
-    api.unsubscribe('eth_blockNumber', subscriptionId);
+    api.unsubscribe(subscriptionId);
   }
 
   render () {

js/src/views/Contract/contract.js

@@ -87,7 +87,7 @@ class Contract extends Component {
     const { api } = this.context;
     const { subscriptionId, blockSubscriptionId, contract } = this.state;
 
-    api.unsubscribe('eth_blockNumber', blockSubscriptionId);
+    api.unsubscribe(blockSubscriptionId);
     contract.unsubscribe(subscriptionId);
   }