Cleanup/follow up work on `RPC`

RPC:
* added `@deprecated` statements and additional comments
* removed `this._destroyed` for the listener checks, because the
  listener is removed when called `destroy()`.
* added `this._port.close()` in addition to removing the listner.

`bridge-test`:
* refactor all the `function () {}` to arrow functions

`frame-rpc-test.js`:
* added these tests to check the `MessageChannel` functionality.

src/shared/frame-rpc.js

@@ -66,9 +66,15 @@ export class RPC {
    * `MessageChannel` are unidirectional we ignore the `sourceFrame` and `origin`
    * properties, in this case.
    *
-   * @param {Window} sourceFrame
+   * TODO: July 2021, currently this class is a bit of a _frankenstein_ because
+   * we support both, `Window.postMessage` and `MessagePort.postMessage`.
+   * Once we move all the inter-frame communication to `MessageChannel` we will
+   * be able to cleanup this class. We have added `@deprecated` statements in
+   * the pieces of code that need to be removed.
+   *
+   * @param {Window} sourceFrame -- @deprecated
    * @param {Window|MessagePort} destFrameOrPort
-   * @param {string} origin - Origin of destination frame
+   * @param {string} origin - Origin of destination frame @deprecated
    * @param {Record<string, (...args: any[]) => void>} methods - Map of method
    *   name to method handler
    */
@@ -78,9 +84,11 @@ export class RPC {
     if (destFrameOrPort instanceof MessagePort) {
       this._port = destFrameOrPort;
     } else {
+      /** @deprecated */
       this.destFrame = destFrameOrPort;
     }
 
+    /** @deprecated */
     if (origin === '*') {
       this.origin = '*';
     } else {
@@ -91,6 +99,7 @@ export class RPC {
 
     this._sequence = 0;
     this._callbacks = {};
+    this._destroyed = false;
 
     this._listeners = new ListenerCollection();
 
@@ -100,6 +109,7 @@ export class RPC {
       );
       this._port.start();
     } else {
+      /** @deprecated */
       /** @param {MessageEvent} event */
       const onmessage = event => {
         if (!this._isValidSender) {
@@ -120,6 +130,9 @@ export class RPC {
   destroy() {
     this._destroyed = true;
     this._listeners.removeAll();
+    if (this._port) {
+      this._port.close();
+    }
   }
 
   /**
@@ -137,8 +150,9 @@ export class RPC {
     }
 
     const seq = this._sequence++;
-    if (typeof args[args.length - 1] === 'function') {
-      this._callbacks[seq] = args[args.length - 1];
+    const finalArg = args[args.length - 1];
+    if (typeof finalArg === 'function') {
+      this._callbacks[seq] = finalArg;
       args = args.slice(0, -1);
     }
 
@@ -154,6 +168,8 @@ export class RPC {
     if (this._port) {
       this._port.postMessage(message);
     }
+
+    /** @deprecated */
     if (this.destFrame) {
       this.destFrame.postMessage(message, this.origin);
     }
@@ -163,13 +179,13 @@ export class RPC {
    * Validate sender
    *
    * @param {MessageEvent} event
+   * @deprecated
    */
   _isValidSender(event) {
-    if (
-      this._destroyed ||
-      this.destFrame !== event.source ||
-      (this.origin !== '*' && event.origin !== this.origin)
-    ) {
+    if (this.destFrame !== event.source) {
+      return false;
+    }
+    if (this.origin !== '*' && event.origin !== this.origin) {
       return false;
     }
 
@@ -182,13 +198,16 @@ export class RPC {
    * @param {MessageEvent} event
    */
   _isValidMessage({ data }) {
-    if (
-      this._destroyed ||
-      !data ||
-      typeof data !== 'object' ||
-      data.protocol !== PROTOCOL ||
-      !Array.isArray(data.arguments)
-    ) {
+    if (!data || typeof data !== 'object') {
+      return false;
+    }
+    if (data.protocol !== PROTOCOL) {
+      return false;
+    }
+    if (data.version !== VERSION) {
+      return false;
+    }
+    if (!Array.isArray(data.arguments)) {
       return false;
     }
 
@@ -224,11 +243,11 @@ export class RPC {
           this._port.postMessage(message);
         }
 
+        /** @deprecated */
         if (this.destFrame) {
           this.destFrame.postMessage(message, this.origin);
         }
       };
-
       this._methods[msg.method].call(this._methods, ...msg.arguments, callback);
     } else if ('response' in msg) {
       const cb = this._callbacks[msg.response];

src/shared/test/bridge-test.js

@@ -115,7 +115,7 @@ describe('shared/bridge', () => {
       sandbox.stub(channel, 'call').throws(new Error(''));
       sandbox.stub(channel, 'destroy');
 
-      const callback = function () {
+      const callback = () => {
         assert.called(channel.destroy);
         done();
       };
@@ -184,9 +184,10 @@ describe('shared/bridge', () => {
       };
 
       const data = {
-        protocol: 'frame-rpc',
-        method: 'connect',
         arguments: ['TOKEN'],
+        method: 'connect',
+        protocol: 'frame-rpc',
+        version: '1.0.0',
       };
 
       const event = {
@@ -212,9 +213,10 @@ describe('shared/bridge', () => {
       };
 
       const data = {
-        protocol: 'frame-rpc',
-        method: 'connect',
         arguments: ['TOKEN'],
+        method: 'connect',
+        protocol: 'frame-rpc',
+        version: '1.0.0',
       };
 
       const event = {

src/shared/test/frame-rpc-test.js

+import { RPC } from '../frame-rpc';
+
+describe('shared/bridge', () => {
+  let port1;
+  let port2;
+  let rpc1;
+  let rpc2;
+  let plusOne;
+
+  beforeEach(() => {
+    const channel = new MessageChannel();
+    port1 = channel.port1;
+    port2 = channel.port2;
+
+    // `concat` method for rpc1
+    const concat = (arg0, ...args) => {
+      const callback = args.pop();
+      const result = arg0.concat(...args);
+      callback(result);
+    };
+
+    rpc1 = new RPC(
+      /** dummy when using ports */ window,
+      port1,
+      /** dummy when using ports */ '*',
+      {
+        concat,
+      }
+    );
+
+    // `plusOne` method for rpc2
+    plusOne = sinon.stub().callsFake((...numbers) => {
+      const callback = numbers.pop();
+      const result = numbers.map(value => value + 1);
+      callback(result);
+    });
+
+    rpc2 = new RPC(
+      /** dummy when using ports */ window,
+      port2,
+      /** dummy when using ports */ '*',
+      {
+        plusOne,
+      }
+    );
+  });
+
+  afterEach(() => {
+    rpc1.destroy();
+    rpc2.destroy();
+  });
+
+  it('should call the method `plusOne` on rpc2', done => {
+    rpc1.call('plusOne', 1, 2, 3, value => {
+      assert.deepEqual(value, [2, 3, 4]);
+      done();
+    });
+  });
+
+  it('should not call the method `plusOne` if rpc1 is destroyed', () => {
+    rpc1.destroy();
+
+    rpc1.call('plusOne', 1, 2, 3);
+    assert.notCalled(plusOne);
+  });
+
+  it('should not call the method `plusOne` if rpc2 is destroyed', done => {
+    rpc2.destroy();
+
+    rpc1.call('plusOne', 1, 2, 3, () => {
+      done(new Error('Unexpected call'));
+    });
+
+    setTimeout(() => {
+      assert.notCalled(plusOne);
+      done();
+    }, 100);
+  });
+
+  it('should call the method `concat` on rpc1', done => {
+    rpc2.call('concat', 'hello', ' ', 'world', value => {
+      assert.equal(value, 'hello world');
+      done();
+    });
+
+    rpc2.call('concat', [1], [2], [3], value => {
+      assert.deepEqual(value, [1, 2, 3]);
+      done();
+    });
+  });
+
+  it('should ignore invalid messages', done => {
+    // Correct message
+    port1.postMessage({
+      arguments: [1, 2],
+      method: 'plusOne',
+      protocol: 'frame-rpc',
+      version: '1.0.0',
+    });
+
+    // Incorrect argument
+    port1.postMessage({
+      arguments: 'test',
+      method: 'plusOne',
+      protocol: 'frame-rpc',
+      version: '1.0.0',
+    });
+
+    // Incorrect method
+    port1.postMessage({
+      arguments: [1, 2],
+      method: 'dummy',
+      protocol: 'frame-rpc',
+      version: '1.0.0',
+    });
+
+    // Incorrect protocol
+    port1.postMessage({
+      arguments: [1, 2],
+      method: 'plusOne',
+      protocol: 'dummy',
+      version: '1.0.0',
+    });
+
+    // Incorrect version
+    port1.postMessage({
+      arguments: [1, 2],
+      method: 'plusOne',
+      protocol: 'frame-rpc',
+      version: 'dummy',
+    });
+
+    // All incorrect
+    port1.postMessage({});
+    port1.postMessage(null);
+    port1.postMessage(undefined);
+    port1.postMessage(0);
+    port1.postMessage('');
+    port1.postMessage('dummy');
+
+    setTimeout(() => {
+      assert.calledOnce(plusOne);
+      done();
+    }, 100);
+  });
+});