Simplify and improve type safety of `watch` utility

Simplify the `watch` utility by making it only support a single callback to get
the watched value, along with an optional comparison function to use to compare
the results. To watch multiple values the callback can return an array and use a
shallow-equality comparison.

In `FrameSyncService` the lengthy callback passed to `watch` was
extracted into a separate function to make the `watch` call more
readable.

src/sidebar/services/frame-sync.js

 import debounce from 'lodash.debounce';
+import shallowEqual from 'shallowequal';
 
 import { ListenerCollection } from '../../shared/listener-collection';
 import { PortFinder, PortRPC, isMessageEqual } from '../../shared/messaging';
@@ -134,86 +135,99 @@ export class FrameSyncService {
   _setupSyncToGuests() {
     let prevPublicAnns = 0;
 
-    watch(
-      this._store.subscribe,
-      [() => this._store.allAnnotations(), () => this._store.frames()],
-      /**
-       * @param {[Annotation[], Frame[]]} current
-       * @param {[Annotation[]]} previous
-       */
-      ([annotations, frames], [prevAnnotations]) => {
-        let publicAnns = 0;
-        /** @type {Set<string>} */
-        const inSidebar = new Set();
-        /** @type {Annotation[]} */
-        const added = [];
-
-        // Determine which annotations have been added or deleted in the sidebar.
-        annotations.forEach(annot => {
-          if (isReply(annot)) {
-            // The frame does not need to know about replies
-            return;
-          }
+    /**
+     * Handle annotations or frames being added or removed in the store.
+     *
+     * @param {Annotation[]} annotations
+     * @param {Frame[]} frames
+     * @param {Annotation[]} prevAnnotations
+     */
+    const onStoreAnnotationsChanged = (
+      annotations,
+      frames,
+      prevAnnotations
+    ) => {
+      let publicAnns = 0;
+      /** @type {Set<string>} */
+      const inSidebar = new Set();
+      /** @type {Annotation[]} */
+      const added = [];
+
+      // Determine which annotations have been added or deleted in the sidebar.
+      annotations.forEach(annot => {
+        if (isReply(annot)) {
+          // The frame does not need to know about replies
+          return;
+        }
 
-          if (isPublic(annot)) {
-            ++publicAnns;
-          }
+        if (isPublic(annot)) {
+          ++publicAnns;
+        }
 
-          inSidebar.add(annot.$tag);
-          if (!this._inFrame.has(annot.$tag)) {
-            added.push(annot);
-          }
-        });
-        const deleted = prevAnnotations.filter(
-          annot => !inSidebar.has(annot.$tag)
-        );
-
-        // Send added annotations to matching frame.
-        if (added.length > 0) {
-          /** @type {Map<string|null, Annotation[]>} */
-          const addedByFrame = new Map();
-          for (let annotation of added) {
-            const frame = frameForAnnotation(frames, annotation);
-            if (!frame) {
-              continue;
-            }
-            const anns = addedByFrame.get(frame.id) ?? [];
-            anns.push(annotation);
-            addedByFrame.set(frame.id, anns);
+        inSidebar.add(annot.$tag);
+        if (!this._inFrame.has(annot.$tag)) {
+          added.push(annot);
+        }
+      });
+      const deleted = prevAnnotations.filter(
+        annot => !inSidebar.has(annot.$tag)
+      );
+
+      // Send added annotations to matching frame.
+      if (added.length > 0) {
+        /** @type {Map<string|null, Annotation[]>} */
+        const addedByFrame = new Map();
+        for (let annotation of added) {
+          const frame = frameForAnnotation(frames, annotation);
+          if (!frame) {
+            continue;
           }
+          const anns = addedByFrame.get(frame.id) ?? [];
+          anns.push(annotation);
+          addedByFrame.set(frame.id, anns);
+        }
 
-          for (let [frameId, anns] of addedByFrame) {
-            const rpc = this._guestRPC.get(frameId);
-            if (rpc) {
-              rpc.call('loadAnnotations', anns.map(formatAnnot));
-            }
+        for (let [frameId, anns] of addedByFrame) {
+          const rpc = this._guestRPC.get(frameId);
+          if (rpc) {
+            rpc.call('loadAnnotations', anns.map(formatAnnot));
           }
-
-          added.forEach(annot => {
-            this._inFrame.add(annot.$tag);
-          });
         }
 
-        // Remove deleted annotations from frames.
-        deleted.forEach(annot => {
-          // Delete from all frames. If a guest is not displaying a particular
-          // annotation, it will just ignore the request.
-          this._guestRPC.forEach(rpc =>
-            rpc.call('deleteAnnotation', annot.$tag)
-          );
-          this._inFrame.delete(annot.$tag);
+        added.forEach(annot => {
+          this._inFrame.add(annot.$tag);
         });
+      }
+
+      // Remove deleted annotations from frames.
+      deleted.forEach(annot => {
+        // Delete from all frames. If a guest is not displaying a particular
+        // annotation, it will just ignore the request.
+        this._guestRPC.forEach(rpc => rpc.call('deleteAnnotation', annot.$tag));
+        this._inFrame.delete(annot.$tag);
+      });
 
-        // Update elements in host page which display annotation counts.
-        if (frames.length > 0) {
-          if (frames.every(frame => frame.isAnnotationFetchComplete)) {
-            if (publicAnns === 0 || publicAnns !== prevPublicAnns) {
-              this._hostRPC.call('publicAnnotationCountChanged', publicAnns);
-              prevPublicAnns = publicAnns;
-            }
+      // Update elements in host page which display annotation counts.
+      if (frames.length > 0) {
+        if (frames.every(frame => frame.isAnnotationFetchComplete)) {
+          if (publicAnns === 0 || publicAnns !== prevPublicAnns) {
+            this._hostRPC.call('publicAnnotationCountChanged', publicAnns);
+            prevPublicAnns = publicAnns;
           }
         }
       }
+    };
+
+    watch(
+      this._store.subscribe,
+      () =>
+        /** @type {const} */ ([
+          this._store.allAnnotations(),
+          this._store.frames(),
+        ]),
+      ([annotations, frames], [prevAnnotations]) =>
+        onStoreAnnotationsChanged(annotations, frames, prevAnnotations),
+      shallowEqual
     );
   }
 

src/sidebar/services/groups.js

+import shallowEqual from 'shallowequal';
+
 import { serviceConfig } from '../config/service-config';
 import { isReply } from '../helpers/annotation-metadata';
 import { combineGroups } from '../helpers/groups';
@@ -157,9 +159,7 @@ export class GroupsService {
     watch(
       this._store.subscribe,
       () => this._mainURI(),
-      () => {
-        this.load();
-      }
+      () => this.load()
     );
 
     // Reload groups when user ID changes. This is a bit inefficient since it
@@ -167,17 +167,19 @@ export class GroupsService {
     // logging in or logging out.
     watch(
       this._store.subscribe,
-      [
-        () => this._store.hasFetchedProfile(),
-        () => this._store.profile().userid,
-      ],
+      () =>
+        /** @type {const} */ ([
+          this._store.hasFetchedProfile(),
+          this._store.profile().userid,
+        ]),
       (_, [prevFetchedProfile]) => {
         if (!prevFetchedProfile) {
           // Ignore the first time that the profile is loaded.
           return;
         }
         this.load();
-      }
+      },
+      shallowEqual
     );
   }
 

src/sidebar/services/streamer.js

@@ -268,9 +268,7 @@ export class StreamerService {
       watch(
         this._store.subscribe,
         () => this._store.profile().userid,
-        () => {
-          this._reconnect();
-        }
+        () => this._reconnect()
       );
     }
 

src/sidebar/util/test/watch-test.js

@@ -47,45 +47,42 @@ describe('sidebar/util/watch', () => {
       assert.notCalled(callback);
     });
 
-    it('compares watched values using strict equality', () => {
+    it('compares watched values using strict equality by default', () => {
       const callback = sinon.stub();
       const store = counterStore();
+      const getValue = () => [store.getState().a];
 
-      const newEmptyObject = () => ({});
-      watch(store.subscribe, newEmptyObject, callback);
+      watch(store.subscribe, getValue, callback);
 
-      store.dispatch({ type: 'INCREMENT_A' });
-      store.dispatch({ type: 'INCREMENT_A' });
+      store.dispatch({ type: 'INCREMENT_B' });
+      store.dispatch({ type: 'INCREMENT_B' });
 
-      // This will trigger the callback because we're comparing values by
-      // strict equality rather than by shallow equality.
+      // The callback is called twice even though `getValue` returns an array
+      // with the same content each time, because a strict equality check is
+      // used.
       assert.calledTwice(callback);
-      assert.calledWith(callback, {});
     });
 
-    it('runs callback if any of multiple watched values changes', () => {
+    it('compares watched values using custom equality check', () => {
       const callback = sinon.stub();
       const store = counterStore();
+      const equals = sinon.stub().returns(true);
 
-      watch(
-        store.subscribe,
-        [() => store.getState().a, () => store.getState().b],
-        callback
-      );
+      watch(store.subscribe, () => store.getState().a, callback, equals);
 
-      // Dispatch action that changes the first watched value.
       store.dispatch({ type: 'INCREMENT_A' });
-      assert.calledWith(callback, [1, 0], [0, 0]);
-
-      // Dispatch action that changes the second watched value.
-      callback.resetHistory();
-      store.dispatch({ type: 'INCREMENT_B' });
-      assert.calledWith(callback, [1, 1], [1, 0]);
+      store.dispatch({ type: 'INCREMENT_A' });
 
-      // Dispatch action that doesn't change either watched value.
-      callback.resetHistory();
-      store.dispatch({ type: 'INCREMENT_C' });
+      assert.calledTwice(equals);
+      assert.calledWith(equals, 1, 0);
+      assert.calledWith(equals, 2, 0);
       assert.notCalled(callback);
+
+      equals.returns(false);
+
+      store.dispatch({ type: 'INCREMENT_A' });
+      assert.calledWith(equals, 3, 0);
+      assert.calledWith(callback, 3, 0);
     });
 
     it('returns unsubscription function', () => {

src/sidebar/util/watch.js

-import shallowEqual from 'shallowequal';
-
 /**
- * Watch for changes of computed values.
+ * Watch a data source for changes to a subset of its data.
+ *
+ * `watch` subscribes to change notifications from a data source, computes
+ * values using the data in it, and runs a callback with the current and
+ * previous computed values each time the computed result changes.
  *
- * This utility is a shorthand for a common pattern for reacting to changes in
- * some data source:
+ * @example
+ *   const unsubscribe = watch(
+ *     store.subscribe,
  *
- * ```
- * let prevValue = getCurrentValue();
- * subscribe(() => {
- *   const newValue = getCurrentValue();
- *   if (prevValue !== newValue) {
- *     // Respond to change of value.
- *     // ...
+ *     // Extract some data of interest from the store.
+ *     () => store.getValue(),
  *
- *     // Update previous value.
- *     prevValue = new value;
- *   }
- * });
- * ```
+ *     // Use strict comparison of values
+ *     null,
  *
- * Where `getCurrentValue` calculates the value of interest and
- * `subscribe` registers a callback to receive change notifications for
- * whatever data source (eg. a Redux store) is used by `getCurrentValue`.
+ *     // Callback that is invoked each time the extracted data changes.
+ *     (currentValue, prevValue) => { ... }
+ *   );
+ *   unsubcribe(); // Remove the subscription
  *
- * With the `watch` utility this becomes:
+ * To watch multiple values, make {@link getValue} return an array and set
+ * {@link compare} to a function that compares each element of the array:
  *
- * ```
- * watch(subscribe, getCurrentValue, (newValue, prevValue) => {
- *   // Respond to change of value
- * });
- * ```
+ * @example
+ *   watch(
+ *     store.subscribe,
+ *     () => [store.getValueA(), store.getValueB()],
  *
- * `watch` can watch a single value, if the second argument is a function,
- * or many if the second argument is an array of functions. In the latter case
- * the callback will be invoked whenever _any_ of the watched values changes.
+ *     // Compare each element of the result
+ *     shallowEqual,
  *
- * Values are compared using strict equality (`===`).
+ *     ([currentValueA, currentValueB], [prevValueA, prevValueB]) => { ... }
+ *   );
  *
- * @param {(callback: () => void) => Function} subscribe - Function used to
- *   subscribe to notifications of _potential_ changes in the watched values.
- * @param {Function|Array<Function>} watchFns - A function or array of functions
- *   which return the current watched values
- * @param {(current: any, previous: any) => void} callback -
- *   A callback that is invoked when the watched values changed. It is passed
- *   the current and previous values respectively. If `watchFns` is an array,
- *   the `current` and `previous` arguments will be arrays of current and
- *   previous values.
- * @return {Function} - Return value of `subscribe`. Typically this is a
- *   function that removes the subscription.
+ * @template T
+ * @param {(callback: () => void) => VoidFunction} subscribe - Function to
+ *   subscribe to changes from the data source.
+ * @param {() => T} getValue - Callback that extracts information of interest
+ *   from the data source.
+ * @param {(current: T, previous: T) => void} callback -
+ *   A callback that receives the data extracted by `getValue`. It is called
+ *   each time the result of `getValue` changes.
+ * @param {((current: T, previous: T) => boolean)} [compare] -
+ *   Comparison function that tests whether the results of two `getValue` calls
+ *   are equal. If omitted, a strict equality check is used
+ * @return {VoidFunction} - Return value of `subscribe`
  */
-export function watch(subscribe, watchFns, callback) {
-  const isArray = Array.isArray(watchFns);
-
-  const getWatchedValues = () =>
-    isArray
-      ? /** @type {Function[]} */ (watchFns).map(fn => fn())
-      : /** @type {Function} */ (watchFns)();
-
-  let prevValues = getWatchedValues();
+export function watch(subscribe, getValue, callback, compare) {
+  let prevValue = getValue();
   const unsubscribe = subscribe(() => {
-    const values = getWatchedValues();
-
-    const equal = isArray
-      ? shallowEqual(values, prevValues)
-      : values === prevValues;
-
-    if (equal) {
+    const currentValue = getValue();
+    if (
+      compare ? compare(currentValue, prevValue) : currentValue === prevValue
+    ) {
       return;
     }
 
     // Save and then update `prevValues` before invoking `callback` in case
     // `callback` triggers another update.
-    const savedPrevValues = prevValues;
-    prevValues = values;
+    const savedPrevValue = prevValue;
+    prevValue = currentValue;
 
-    callback(values, savedPrevValues);
+    callback(currentValue, savedPrevValue);
   });
 
   return unsubscribe;