Remove `onConnect` method from Bridge

Simplify the Bridge class by removing the `onConnect` API and replacing
current calls with alternative approaches.

The `onConnect` method was a way to register a callback that would be invoked
after a test RPC call over a newly-connected channel succeeded.

The purpose of this method was not that clear however and its existence
suggested that it is necessary to wait for the callback to be invoked before
making RPC requests on the new channel. This is not the case, as the underlying
transport buffers messages until the receiver is ready to consume them. Therefore
RPC requests can be made as soon as `createChannel` returns.

The current uses of `onConnect` were replaced as follows:

 1. The host frame used it to be notified when the sidebar application has
    loaded. This has been replaced with an explicit `ready` RPC call.

 2. The sidebar waited for this callback before querying metadata for a
    newly-connected guest. It now sends a metadata query to the guest
    immediately when it receives the guest port from the host frame. This saves
    an unnecessary round-trip between the host and guest. In future we can
    optimize metadata querying more by having the guest _push_ metadata to
    the sidebar rather than waiting for the sidebar to query it.

src/annotator/sidebar.js

@@ -189,7 +189,7 @@ export default class Sidebar {
     this._notifyOfLayoutChange(false);
     this._setupSidebarEvents();
 
-    this._sidebarRPC.onConnect(() => {
+    this._sidebarRPC.on('ready', () => {
       // Show the UI
       if (this.iframeContainer) {
         this.iframeContainer.style.display = '';

src/annotator/test/guest-test.js

@@ -106,7 +106,6 @@ describe('Guest', () => {
         createChannel: sinon.stub(),
         destroy: sinon.stub(),
         on: sinon.stub(),
-        onConnect: sinon.stub(),
       };
       fakeBridges.push(bridge);
       return bridge;

src/annotator/test/sidebar-test.js

@@ -72,8 +72,7 @@ describe('Sidebar', () => {
    * when the sidebar has loaded and is ready.
    */
   const connectSidebarApp = () => {
-    const callback = sidebarBridge().onConnect.getCall(0).args[0];
-    callback();
+    emitSidebarEvent('ready');
   };
 
   const createSidebar = (config = {}) => {
@@ -116,7 +115,6 @@ describe('Sidebar', () => {
         createChannel: sinon.stub(),
         destroy: sinon.stub(),
         on: sinon.stub(),
-        onConnect: sinon.stub(),
       };
       fakeBridges.push(bridge);
       return bridge;

src/shared/bridge.js

@@ -19,12 +19,6 @@ export class Bridge {
     this.links = [];
     /** @type {Record<string, (...args: any[]) => void>} */
     this.channelListeners = {};
-    /** @type {Array<(channel: PortRPC) => void>} */
-    this.onConnectListeners = [];
-
-    // `connect` is registered with a callback so it triggers a `postMessage`
-    // response from the reciprocal port.
-    this.on('connect', cb => cb());
   }
 
   /**
@@ -48,22 +42,7 @@ export class Bridge {
    */
   createChannel(port) {
     const channel = new PortRPC(port, this.channelListeners);
-
-    let connected = false;
-    const ready = () => {
-      if (connected) {
-        return;
-      }
-      connected = true;
-      this.onConnectListeners.forEach(cb => cb(channel));
-    };
-
-    // Fire off a connection attempt
-    channel.call('connect', ready);
-
-    // Store the newly created channel in our collection
     this.links.push(channel);
-
     return channel;
   }
 
@@ -157,14 +136,4 @@ export class Bridge {
     this.channelListeners[method] = listener;
     return this;
   }
-
-  /**
-   * Add a listener to be called upon a new connection.
-   *
-   * @param {(channel: PortRPC) => void} listener
-   */
-  onConnect(listener) {
-    this.onConnectListeners.push(listener);
-    return this;
-  }
 }

src/shared/test/bridge-test.js

@@ -194,59 +194,6 @@ describe('shared/bridge', () => {
     });
   });
 
-  describe('#onConnect', () => {
-    // Simulate a Bridge attached to the other end of a channel receiving
-    // the `connect` RPC request and handling it using the `connect` handler
-    // registered by the Bridge.
-    const runConnectHandler = channel => {
-      const connectCall = channel.call
-        .getCalls()
-        .find(call => call.firstArg === 'connect');
-
-      // Invoke the `connect` handler. Here we're invoking it on `channel` but
-      // in the actual app this would be called on the counterpart channel in
-      // the other frame.
-      channel.methods.connect(...connectCall.args.slice(1));
-    };
-
-    it('runs callbacks when channel connects', () => {
-      const onConnectCallback = sinon.stub();
-      bridge.onConnect(onConnectCallback);
-
-      const channel = createChannel();
-
-      runConnectHandler(channel);
-
-      assert.calledWith(onConnectCallback, channel);
-    });
-
-    it('allows multiple callbacks to be registered', () => {
-      const onConnectCallback1 = sinon.stub();
-      const onConnectCallback2 = sinon.stub();
-      bridge.onConnect(onConnectCallback1);
-      bridge.onConnect(onConnectCallback2);
-
-      const channel = createChannel();
-
-      runConnectHandler(channel);
-
-      assert.calledWith(onConnectCallback1, channel);
-      assert.calledWith(onConnectCallback2, channel);
-    });
-
-    it('only invokes `onConnect` callback once', () => {
-      const onConnectCallback = sinon.stub();
-      bridge.onConnect(onConnectCallback);
-
-      const channel = createChannel();
-
-      runConnectHandler(channel);
-      runConnectHandler(channel);
-
-      assert.calledOnce(onConnectCallback);
-    });
-  });
-
   describe('#destroy', () =>
     it('destroys all opened channels', () => {
       const channel1 = bridge.createChannel();

src/shared/test/integration/rpc-bridge-test.js

@@ -24,34 +24,6 @@ describe('PortRPC-Bridge integration', () => {
     clock.restore();
   });
 
-  describe('establishing a connection', () => {
-    it('should invoke Bridge `onConnect` callbacks after connecting', async () => {
-      const bridge = createBridge();
-      const reciprocalBridge = createBridge();
-      reciprocalBridge.on('method', cb => cb(null));
-      let callbackCount = 0;
-      const callback = () => {
-        ++callbackCount;
-      };
-      bridge.onConnect(callback);
-      bridge.onConnect(callback); // allows multiple callbacks to be registered
-      reciprocalBridge.onConnect(callback);
-
-      const channel = bridge.createChannel(port1);
-      const reciprocalChannel = reciprocalBridge.createChannel(port2);
-      await bridge.call('method');
-
-      assert.equal(callbackCount, 3);
-
-      // Additional calls to the RPC `connect` method are ignored
-      await channel.call('connect');
-      await reciprocalChannel.call('connect');
-      await bridge.call('method');
-
-      assert.equal(callbackCount, 3);
-    });
-  });
-
   describe('sending and receiving RPC messages', () => {
     it('should invoke Bridge method handler on every channel when calling a RPC method', async () => {
       const bridge = createBridge();

src/sidebar/services/frame-sync.js

@@ -250,32 +250,30 @@ export class FrameSyncService {
     this._guestRPC.on('closeSidebar', () => {
       this._hostRPC.call('closeSidebar');
     });
+  }
 
-    /**
-     * Query the guest in a frame for the URL and metadata of the document that
-     * is currently loaded and add the result to the set of connected frames.
-     *
-     * @param {PortRPC} channel
-     */
-    const addFrame = channel => {
-      // Synchronize highlight visibility in this guest with the sidebar's controls.
-      channel.call('setHighlightsVisible', this._highlightsVisible);
-
-      channel.call('getDocumentInfo', (err, info) => {
-        if (err) {
-          channel.destroy();
-          return;
-        }
+  /**
+   * Query the guest in a frame for the URL and metadata of the document that
+   * is currently loaded and add the result to the set of connected frames.
+   *
+   * @param {PortRPC} channel
+   */
+  _addFrame(channel) {
+    // Synchronize highlight visibility in this guest with the sidebar's controls.
+    channel.call('setHighlightsVisible', this._highlightsVisible);
+
+    channel.call('getDocumentInfo', (err, info) => {
+      if (err) {
+        channel.destroy();
+        return;
+      }
 
-        this._store.connectFrame({
-          id: info.frameIdentifier,
-          metadata: info.metadata,
-          uri: info.uri,
-        });
+      this._store.connectFrame({
+        id: info.frameIdentifier,
+        metadata: info.metadata,
+        uri: info.uri,
       });
-    };
-
-    this._guestRPC.onConnect(addFrame);
+    });
   }
 
   /**
@@ -311,6 +309,7 @@ export class FrameSyncService {
     // Create channel for sidebar-host communication.
     const hostPort = await this._portFinder.discover('host');
     this._hostRPC.createChannel(hostPort);
+    this._hostRPC.call('ready');
 
     // Listen for guests connecting to the sidebar.
     this._listeners.add(hostPort, 'message', event => {
@@ -322,7 +321,8 @@ export class FrameSyncService {
           type: 'offer',
         })
       ) {
-        this._guestRPC.createChannel(ports[0]);
+        const channel = this._guestRPC.createChannel(ports[0]);
+        this._addFrame(channel);
       }
     });
   }

src/sidebar/services/test/frame-sync-test.js

@@ -60,6 +60,7 @@ describe('FrameSyncService', () => {
   let fakeAnnotationsService;
   let fakeBridges;
   let fakePortFinder;
+
   let fakeStore;
   let fakeWindow;
 
@@ -67,20 +68,31 @@ describe('FrameSyncService', () => {
   let hostPort;
   let sidebarPort;
 
+  // Hook to prepare channels created by `Bridge.createChannel` before they are
+  // returned to the service.
+  let setupChannel;
+
   beforeEach(() => {
     fakeAnnotationsService = { create: sinon.stub() };
-
     fakeBridges = [];
+    setupChannel = null;
+
     FakeBridge = sinon.stub().callsFake(() => {
       const emitter = new EventEmitter();
       const bridge = {
         call: sinon.stub(),
-        createChannel: sinon.stub(),
+        createChannel: sinon.stub().callsFake(() => {
+          const rpc = {
+            call: sinon.stub(),
+            destroy: sinon.stub(),
+          };
+
+          setupChannel?.(rpc);
+
+          return rpc;
+        }),
         emit: emitter.emit.bind(emitter),
         on: emitter.on.bind(emitter),
-        onConnect: function (listener) {
-          emitter.on('connect', listener);
-        },
       };
       fakeBridges.push(bridge);
       return bridge;
@@ -160,6 +172,25 @@ describe('FrameSyncService', () => {
     guestBridge().emit(event, ...args);
   }
 
+  /**
+   * Simulate a new guest frame connecting to the sidebar.
+   *
+   * @return {MessagePort} - The port that was sent to the sidebar
+   */
+  async function connectGuest() {
+    const { port1 } = new MessageChannel();
+    hostPort.postMessage(
+      {
+        frame1: 'guest',
+        frame2: 'sidebar',
+        type: 'offer',
+      },
+      [port1]
+    );
+    await delay(0);
+    return port1;
+  }
+
   describe('#connect', () => {
     it('discovers and connects to the host frame', async () => {
       await frameSync.connect();
@@ -167,21 +198,16 @@ describe('FrameSyncService', () => {
       assert.calledWith(hostBridge().createChannel, sidebarPort);
     });
 
-    it('connects to new guests when they are ready', async () => {
-      const { port1 } = new MessageChannel();
+    it('notifies the host frame that the sidebar is ready to be displayed', async () => {
+      await frameSync.connect();
 
-      frameSync.connect();
-      hostPort.postMessage(
-        {
-          frame1: 'guest',
-          frame2: 'sidebar',
-          type: 'offer',
-        },
-        [port1]
-      );
-      await delay(0);
+      assert.calledWith(hostBridge().call, 'ready');
+    });
 
-      assert.calledWith(guestBridge().createChannel, port1);
+    it('connects to new guests', async () => {
+      frameSync.connect();
+      const port = await connectGuest();
+      assert.calledWith(guestBridge().createChannel, port);
     });
   });
 
@@ -419,25 +445,19 @@ describe('FrameSyncService', () => {
   });
 
   context('when a new frame connects', () => {
-    let frameInfo;
-    let fakeChannel;
-
     beforeEach(() => {
-      fakeChannel = {
-        call: sinon.spy((name, callback) => {
-          if (name === 'getDocumentInfo') {
-            callback(null, frameInfo);
-          }
-        }),
-        destroy: sinon.stub(),
-      };
       frameSync.connect();
     });
 
-    it("adds the page's metadata to the frames list", () => {
-      frameInfo = fixtures.htmlDocumentInfo;
+    it("adds the page's metadata to the frames list", async () => {
+      const frameInfo = fixtures.htmlDocumentInfo;
+      setupChannel = rpc => {
+        rpc.call.withArgs('getDocumentInfo').callsFake((method, callback) => {
+          callback(null, frameInfo);
+        });
+      };
 
-      emitGuestEvent('connect', fakeChannel);
+      await connectGuest();
 
       assert.calledWith(fakeStore.connectFrame, {
         id: frameInfo.frameIdentifier,
@@ -446,27 +466,35 @@ describe('FrameSyncService', () => {
       });
     });
 
-    it('closes the channel and does not add frame to store if getting document info fails', () => {
-      fakeChannel.call = (name, callback) => {
-        if (name === 'getDocumentInfo') {
-          callback('Something went wrong');
-        }
+    it('closes the channel and does not add frame to store if getting document info fails', async () => {
+      let channel;
+      setupChannel = rpc => {
+        rpc.call.withArgs('getDocumentInfo').callsFake((method, callback) => {
+          callback('Error getting document info');
+        });
+        channel = rpc;
       };
 
-      emitGuestEvent('connect', fakeChannel);
+      await connectGuest();
 
-      assert.called(fakeChannel.destroy);
+      assert.ok(channel);
+      assert.called(channel.destroy);
       assert.notCalled(fakeStore.connectFrame);
     });
 
-    it("synchronizes highlight visibility in the guest with the sidebar's controls", () => {
+    it("synchronizes highlight visibility in the guest with the sidebar's controls", async () => {
+      let channel;
+      setupChannel = rpc => {
+        channel = rpc;
+      };
+
       emitHostEvent('setHighlightsVisible', true);
-      emitGuestEvent('connect', fakeChannel);
-      assert.calledWith(fakeChannel.call, 'setHighlightsVisible', true);
+      await connectGuest();
+      assert.calledWith(channel.call, 'setHighlightsVisible', true);
 
       emitHostEvent('setHighlightsVisible', false);
-      emitGuestEvent('connect', fakeChannel);
-      assert.calledWith(fakeChannel.call, 'setHighlightsVisible', false);
+      await connectGuest();
+      assert.calledWith(channel.call, 'setHighlightsVisible', false);
     });
   });
 

src/types/bridge-events.d.ts

@@ -137,6 +137,11 @@ export type SidebarToGuestEvent =
  * Events that the sidebar sends to the host
  */
 export type SidebarToHostEvent =
+  /**
+   * The sidebar notifies the host that it has loaded and is ready to be displayed.
+   */
+  | 'ready'
+
   /**
    * The sidebar relays to the host to close the sidebar.
    */