Merge pull request #2038 from hypothesis/remove-frame-connected

Remove `FRAME_CONNECTED` event

src/sidebar/events.js

@@ -3,9 +3,6 @@
  * on $rootScope
  */
 export default {
-  // Internal state changes
-  FRAME_CONNECTED: 'frameConnected',
-
   // Session state changes
 
   /** The logged-in user changed */

src/sidebar/services/features.js

@@ -12,23 +12,20 @@
 
 import bridgeEvents from '../../shared/bridge-events';
 import warnOnce from '../../shared/warn-once';
-import events from '../events';
+import { watch } from '../util/watch';
 
 // @ngInject
-export default function features($rootScope, bridge, session) {
-  const _sendFeatureFlags = function () {
-    const userFeatures = session.state.features;
-    bridge.call(bridgeEvents.FEATURE_FLAGS_UPDATED, userFeatures || {});
+export default function features(bridge, session, store) {
+  const currentFlags = () => store.profile().features;
+  const sendFeatureFlags = () => {
+    bridge.call(bridgeEvents.FEATURE_FLAGS_UPDATED, currentFlags() || {});
   };
 
-  // user changed is currently called when we initially load
-  // the sidebar and when the user actually logs out/in.
-  $rootScope.$on(events.USER_CHANGED, _sendFeatureFlags);
-
-  // send on frame connected as well because the user_changed event
-  // alone might run before the frames ever connected. This will
-  // provide us the follow up to make sure that the frames get the flags
-  $rootScope.$on(events.FRAME_CONNECTED, _sendFeatureFlags);
+  // Re-send feature flags to connected frames when flags change or a new
+  // frame connects.
+  watch(store.subscribe, [currentFlags, () => store.frames()], () =>
+    sendFeatureFlags()
+  );
 
   /**
    * Returns true if the flag with the given name is enabled for the current
@@ -43,17 +40,18 @@ export default function features($rootScope, bridge, session) {
     // (see CACHE_TTL in session.js)
     session.load();
 
-    if (!session.state.features) {
+    const flags = currentFlags();
+
+    if (!flags) {
       // features data has not yet been fetched
       return false;
     }
 
-    const features = session.state.features;
-    if (!(flag in features)) {
+    if (!(flag in flags)) {
       warnOnce('looked up unknown feature', flag);
       return false;
     }
-    return features[flag];
+    return flags[flag];
   }
 
   return {

src/sidebar/services/frame-sync.js

@@ -203,7 +203,6 @@ export default function FrameSync($rootScope, $window, store, bridge) {
         return;
       }
 
-      $rootScope.$broadcast(events.FRAME_CONNECTED);
       store.connectFrame({
         id: info.frameIdentifier,
         metadata: info.metadata,

src/sidebar/services/groups.js

@@ -3,6 +3,7 @@ import serviceConfig from '../service-config';
 import { isReply } from '../util/annotation-metadata';
 import { combineGroups } from '../util/groups';
 import { awaitStateChange } from '../util/state';
+import { watch } from '../util/watch';
 
 const DEFAULT_ORG_ID = '__default__';
 
@@ -31,14 +32,19 @@ export default function groups(
   const svc = serviceConfig(settings);
   const authority = svc ? svc.authority : null;
 
-  function getDocumentUriForGroupSearch() {
-    function mainUri() {
-      const mainFrame = store.mainFrame();
-      if (!mainFrame) {
-        return null;
-      }
-      return mainFrame.uri;
+  /**
+   * Return the main document URI that is used to fetch groups associated with
+   * the site that the user is on.
+   */
+  function mainUri() {
+    const mainFrame = store.mainFrame();
+    if (!mainFrame) {
+      return null;
     }
+    return mainFrame.uri;
+  }
+
+  function getDocumentUriForGroupSearch() {
     return awaitStateChange(store, mainUri);
   }
 
@@ -167,11 +173,7 @@ export default function groups(
     });
   }
 
-  // The document URI passed to the most recent `GET /api/groups` call in order
-  // to include groups associated with this page. This is retained to determine
-  // whether we need to re-fetch groups if the URLs of frames connected to the
-  // sidebar app changes.
-  let documentUri = null;
+  let didSubscribeToUriChanges = false;
 
   /*
    * Fetch an individual group.
@@ -193,15 +195,27 @@ export default function groups(
    * The groups that are fetched depend on the current user, the URI of
    * the current document, and the direct-linked group and/or annotation.
    *
+   * On startup, `load()` must be called to trigger the initial groups fetch.
+   * Subsequently groups are automatically reloaded if the logged-in user or
+   * main document URI changes.
+   *
    * @return {Promise<Group[]>}
    */
   async function load() {
     // Step 1: Get the URI of the active document, so we can fetch groups
     // associated with that document.
+    let documentUri;
     if (isSidebar) {
       documentUri = await getDocumentUriForGroupSearch();
     }
 
+    if (!didSubscribeToUriChanges) {
+      didSubscribeToUriChanges = true;
+      watch(store.subscribe, mainUri, () => {
+        load();
+      });
+    }
+
     // Step 2: Concurrently fetch the groups the user is a member of,
     // the groups associated with the current document and the annotation
     // and/or group that was direct-linked (if any).
@@ -362,16 +376,6 @@ export default function groups(
     return load();
   });
 
-  // refetch the list of groups when document url changes
-  $rootScope.$on(events.FRAME_CONNECTED, () => {
-    return getDocumentUriForGroupSearch().then(uri => {
-      if (documentUri !== uri) {
-        documentUri = uri;
-        load();
-      }
-    });
-  });
-
   return {
     all: all,
     get: get,

src/sidebar/services/test/features-test.js

 import bridgeEvents from '../../../shared/bridge-events';
-import events from '../../events';
 import features from '../features';
 import { $imports } from '../features';
 
-describe('h:features - sidebar layer', function () {
+describe('sidebar/services/features', function () {
   let fakeBridge;
   let fakeWarnOnce;
-  let fakeRootScope;
   let fakeSession;
-  let sandbox;
+  let fakeStore;
 
   beforeEach(function () {
-    sandbox = sinon.createSandbox();
-
     fakeBridge = {
       call: sinon.stub(),
     };
 
     fakeWarnOnce = sinon.stub();
 
-    fakeRootScope = {
-      eventCallbacks: {},
-
-      $broadcast: sandbox.stub(),
-
-      $on: function (event, callback) {
-        this.eventCallbacks[event] = callback;
-      },
-    };
-
     fakeSession = {
       load: sinon.stub(),
-      state: {
+    };
+
+    fakeStore = {
+      subscribe: sinon.stub(),
+      frames: sinon.stub().returns([]),
+      profile: sinon.stub().returns({
         features: {
           feature_on: true,
           feature_off: false,
         },
-      },
+      }),
     };
 
     $imports.$mock({
@@ -46,64 +37,83 @@ describe('h:features - sidebar layer', function () {
 
   afterEach(function () {
     $imports.$restore();
-    sandbox.restore();
   });
 
+  function createService() {
+    return features(fakeBridge, fakeSession, fakeStore);
+  }
+
   describe('flagEnabled', function () {
     it('should retrieve features data', function () {
-      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
+      const features_ = createService();
       assert.equal(features_.flagEnabled('feature_on'), true);
       assert.equal(features_.flagEnabled('feature_off'), false);
     });
 
     it('should return false if features have not been loaded', function () {
-      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
-      // simulate feature data not having been loaded yet
-      fakeSession.state = {};
+      const features_ = createService();
+      // Simulate feature data not having been loaded yet
+      fakeStore.profile.returns({});
       assert.equal(features_.flagEnabled('feature_on'), false);
     });
 
     it('should trigger a refresh of session data', function () {
-      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
+      const features_ = createService();
       features_.flagEnabled('feature_on');
       assert.calledOnce(fakeSession.load);
     });
 
     it('should return false for unknown flags', function () {
-      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
+      const features_ = createService();
       assert.isFalse(features_.flagEnabled('unknown_feature'));
     });
   });
 
-  it('should broadcast feature flags to annotation layer based on load/user changes', function () {
-    assert.notProperty(fakeRootScope.eventCallbacks, events.USER_CHANGED);
-    assert.notProperty(fakeRootScope.eventCallbacks, events.FRAME_CONNECTED);
+  function notifyStoreSubscribers() {
+    const subscribers = fakeStore.subscribe.args.map(args => args[0]);
+    subscribers.forEach(s => s());
+  }
 
-    features(fakeRootScope, fakeBridge, fakeSession);
+  it('should broadcast feature flags to annotator if flags change', () => {
+    createService();
 
-    assert.property(fakeRootScope.eventCallbacks, events.USER_CHANGED);
-    assert.property(fakeRootScope.eventCallbacks, events.FRAME_CONNECTED);
-
-    // respond to user changing by broadcasting the feature flags
+    // First update, with no changes to feature flags.
+    notifyStoreSubscribers();
     assert.notCalled(fakeBridge.call);
 
-    fakeRootScope.eventCallbacks[events.USER_CHANGED]();
+    // Second update, with changes to feature flags.
+    fakeStore.profile.returns({
+      features: {
+        feature_on: true,
+        feature_off: true,
+      },
+    });
+
+    notifyStoreSubscribers();
 
-    assert.calledOnce(fakeBridge.call);
     assert.calledWith(
       fakeBridge.call,
       bridgeEvents.FEATURE_FLAGS_UPDATED,
-      fakeSession.state.features
+      fakeStore.profile().features
     );
+  });
+
+  it('should broadcast feature flags to annotator if a new frame connects', () => {
+    createService();
+
+    // First update, with no changes to frames.
+    notifyStoreSubscribers();
+    assert.notCalled(fakeBridge.call);
+
+    // Second update, with changes to frames.
+    fakeStore.frames.returns([{ uri: 'https://example.com' }]);
 
-    // respond to frame connections by broadcasting the feature flags
-    fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
+    notifyStoreSubscribers();
 
-    assert.calledTwice(fakeBridge.call);
     assert.calledWith(
       fakeBridge.call,
       bridgeEvents.FEATURE_FLAGS_UPDATED,
-      fakeSession.state.features
+      fakeStore.profile().features
     );
   });
 });

src/sidebar/services/test/groups-test.js

 import events from '../../events';
 import fakeReduxStore from '../../test/fake-redux-store';
 import groups, { $imports } from '../groups';
+import { waitFor } from '../../../test-util/wait';
 
 /**
  * Generate a truth table containing every possible combination of a set of
@@ -108,7 +109,7 @@ describe('groups', function () {
       },
 
       $on: function (event, callback) {
-        if (event === events.USER_CHANGED || event === events.FRAME_CONNECTED) {
+        if (event === events.USER_CHANGED) {
           this.eventCallbacks[event] = callback;
         }
       },
@@ -809,7 +810,7 @@ describe('groups', function () {
     });
   });
 
-  describe('calls load on various events', function () {
+  describe('automatic re-fetching', function () {
     it('refetches groups when the logged-in user changes', () => {
       service();
 
@@ -819,43 +820,46 @@ describe('groups', function () {
     });
 
     context('when a new frame connects', () => {
-      it('should refetch groups if main frame URL has changed', () => {
+      it('should refetch groups if main frame URL has changed', async () => {
         const svc = service();
 
         fakeStore.setState({
           frames: [{ uri: 'https://domain.com/page-a' }],
         });
-        return svc
-          .load()
-          .then(() => {
-            // Simulate main frame URL change, eg. due to client-side navigation in
-            // a single page application.
-            fakeApi.groups.list.resetHistory();
-            fakeStore.setState({
-              frames: [{ uri: 'https://domain.com/page-b' }],
-            });
-
-            return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
-          })
-          .then(() => {
-            assert.calledOnce(fakeApi.groups.list);
-          });
+        await svc.load();
+
+        // Simulate main frame URL change, eg. due to client-side navigation in
+        // a single page application.
+        fakeApi.groups.list.resetHistory();
+        fakeStore.setState({
+          frames: [{ uri: 'https://domain.com/page-b' }],
+        });
+
+        await waitFor(() => fakeApi.groups.list.callCount > 0);
+        assert.calledOnce(fakeApi.groups.list);
       });
 
-      it('should not refetch groups if main frame URL has not changed', () => {
+      it('should not refetch groups if main frame URL has not changed', async () => {
         const svc = service();
 
         fakeStore.setState({
           frames: [{ uri: 'https://domain.com/page-a' }],
         });
-        return svc
-          .load()
-          .then(() => {
-            return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
-          })
-          .then(() => {
-            assert.calledOnce(fakeApi.groups.list);
-          });
+
+        await svc.load();
+        assert.calledOnce(fakeApi.groups.list);
+
+        // A new frame connects, but the main frame URI remains the same.
+        fakeApi.groups.list.resetHistory();
+        fakeStore.setState({
+          frames: [
+            { uri: 'https://domain.com/page-a' },
+            { uri: 'https://domain.com/iframe-b' },
+          ],
+        });
+
+        await new Promise(resolve => setTimeout(resolve, 1));
+        assert.notCalled(fakeApi.groups.list);
       });
     });
   });