Make thread building more idiomatic by using memoization (#3436)

The idiomatic way to create derived data structures from data in the
Redux state store is to do:

  derivedData = transform(select(store.getState()))

Where the `select` function extracts the relevant fields from the state
and the `transform` function then computes the derived data. Both
`select` and `transform` can be trivially memoized to avoid unnecessary
recalculations.

This simplifies `root-thread` by avoiding inheritance from EventEmitter
and in future will make it easier to avoid rebuilding the thread if none
of the relevant application state has changed.

h/static/scripts/annotation-viewer-controller.js

@@ -17,18 +17,18 @@ function AnnotationViewerController (
     $location.path('/stream').search('q', query);
   };
 
-  rootThread.on('changed', function (thread) {
+  function thread() {
+    return rootThread.thread(annotationUI.getState());
+  }
+
+  annotationUI.subscribe(function () {
     $scope.virtualThreadList = {
-      visibleThreads: thread.children,
+      visibleThreads: thread().children,
       offscreenUpperHeight: '0px',
       offscreenLowerHeight: '0px',
     };
   });
 
-  $scope.rootThread = function () {
-    return rootThread.thread();
-  };
-
   $scope.setCollapsed = function (id, collapsed) {
     annotationUI.setCollapsed(id, collapsed);
   };

h/static/scripts/root-thread.js

 'use strict';
 
-var EventEmitter = require('tiny-emitter');
-var inherits = require('inherits');
-
 var buildThread = require('./build-thread');
 var events = require('./events');
+var memoize = require('./util/memoize');
 var metadata = require('./annotation-metadata');
 
 function truthyKeys(map) {
@@ -41,19 +39,18 @@ var sortFns = {
  */
 // @ngInject
 function RootThread($rootScope, annotationUI, searchFilter, viewFilter) {
-  var self = this;
-  var thread;
 
   /**
-   * Rebuild the root conversation thread. This should be called
-   * whenever the set of annotations to render or the sort/search/filter
-   * settings change.
+   * Build the root conversation thread from the given UI state.
+   *
+   * @param state - The current UI state (loaded annotations, sort mode,
+   *        filter settings etc.)
    */
-  function rebuildRootThread() {
-    var sortFn = sortFns[annotationUI.getState().sortKey];
+  function buildRootThread(state) {
+    var sortFn = sortFns[state.sortKey];
 
     var filters;
-    var filterQuery = annotationUI.getState().filterQuery;
+    var filterQuery = state.filterQuery;
 
     if (filterQuery) {
       filters = searchFilter.generateFacetedFilter(filterQuery);
@@ -68,18 +65,14 @@ function RootThread($rootScope, annotationUI, searchFilter, viewFilter) {
 
     // Get the currently loaded annotations and the set of inputs which
     // determines what is visible and build the visible thread structure
-    var state = annotationUI.getState();
-    thread = buildThread(state.annotations, {
+    return buildThread(state.annotations, {
       forceVisible: truthyKeys(state.forceVisible),
       expanded: state.expanded,
       selected: truthyKeys(state.selectedAnnotationMap || {}),
       sortCompareFn: sortFn,
       filterFn: filterFn,
     });
-    self.emit('changed', thread);
   }
-  rebuildRootThread();
-  annotationUI.subscribe(rebuildRootThread);
 
   // Listen for annotations being created or loaded
   // and show them in the UI.
@@ -118,19 +111,10 @@ function RootThread($rootScope, annotationUI, searchFilter, viewFilter) {
   });
 
   /**
-   * Rebuild the conversation thread based on the currently loaded annotations
-   * and search/sort/filter settings.
-   */
-  this.rebuild = rebuildRootThread;
-
-  /**
-   * Returns the current root conversation thread.
+   * Build the root conversation thread from the given UI state.
    * @return {Thread}
    */
-  this.thread = function () {
-    return thread;
-  };
+  this.thread = memoize(buildRootThread);
 }
-inherits(RootThread, EventEmitter);
 
 module.exports = RootThread;

h/static/scripts/stream-controller.coffee

@@ -52,9 +52,12 @@ module.exports = class StreamController
     $scope.forceVisible = (id) ->
       annotationUI.setForceVisible(id, true)
 
-    rootThread.on('changed', (thread) ->
+    thread = ->
+      rootThread.thread(annotationUI.getState())
+
+    annotationUI.subscribe( ->
       $scope.virtualThreadList = {
-        visibleThreads: thread.children,
+        visibleThreads: thread().children,
         offscreenUpperHeight: '0px',
         offscreenLowerHeight: '0px',
       };
@@ -64,6 +67,4 @@ module.exports = class StreamController
     annotationUI.setSortKey('Newest')
 
     $scope.isStream = true
-    $scope.rootThread = ->
-      return rootThread.thread()
     $scope.loadMore = fetch

h/static/scripts/test/annotation-viewer-controller-test.js

@@ -36,7 +36,7 @@ describe('AnnotationViewerController', function () {
       $scope: opts.$scope || {
         search: {},
       },
-      annotationUI: {},
+      annotationUI: {subscribe: sinon.stub()},
       rootThread: new FakeRootThread(),
       streamer: opts.streamer || { setConfig: function () {} },
       store: opts.store || {

h/static/scripts/test/integration/threading-test.js

@@ -52,26 +52,26 @@ describe('annotation threading', function () {
 
   it('should display newly loaded annotations', function () {
     annotationUI.addAnnotations(fixtures.annotations);
-    assert.equal(rootThread.thread().children.length, 2);
+    assert.equal(rootThread.thread(annotationUI.getState()).children.length, 2);
   });
 
   it('should not display unloaded annotations', function () {
     annotationUI.addAnnotations(fixtures.annotations);
     annotationUI.removeAnnotations(fixtures.annotations);
-    assert.equal(rootThread.thread().children.length, 0);
+    assert.equal(rootThread.thread(annotationUI.getState()).children.length, 0);
   });
 
   it('should filter annotations when a search is set', function () {
     annotationUI.addAnnotations(fixtures.annotations);
     annotationUI.setFilterQuery('second');
-    assert.equal(rootThread.thread().children.length, 1);
-    assert.equal(rootThread.thread().children[0].id, '2');
+    assert.equal(rootThread.thread(annotationUI.getState()).children.length, 1);
+    assert.equal(rootThread.thread(annotationUI.getState()).children[0].id, '2');
   });
 
   unroll('should sort annotations by #mode', function (testCase) {
     annotationUI.addAnnotations(fixtures.annotations);
     annotationUI.setSortKey(testCase.sortKey);
-    var actualOrder = rootThread.thread().children.map(function (thread) {
+    var actualOrder = rootThread.thread(annotationUI.getState()).children.map(function (thread) {
       return thread.annotation.id;
     });
     assert.deepEqual(actualOrder, testCase.expectedOrder);

h/static/scripts/test/root-thread-test.js

@@ -77,26 +77,9 @@ describe('rootThread', function () {
     });
   });
 
-  describe('initialization', function () {
-    it('builds a thread from the current set of annotations', function () {
-      assert.equal(rootThread.thread(), fixtures.emptyThread);
-    });
-  });
-
-  function assertRebuildsThread(fn) {
-    fakeBuildThread.reset();
-    var thread = Object.assign({}, fixtures.emptyThread);
-    fakeBuildThread.returns(thread);
-    fn();
-    assert.called(fakeBuildThread);
-    assert.equal(rootThread.thread(), thread);
-  }
-
-  describe('#rebuild', function () {
-    it('rebuilds the thread', function () {
-      assertRebuildsThread(function () {
-        rootThread.rebuild();
-      });
+  describe('#thread', function () {
+    it('returns the result of buildThread()', function() {
+      assert.equal(rootThread.thread(fakeAnnotationUI.state), fixtures.emptyThread);
     });
 
     it('passes loaded annotations to buildThread()', function () {
@@ -104,7 +87,7 @@ describe('rootThread', function () {
       fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, {
         annotations: [annotation],
       });
-      rootThread.rebuild();
+      rootThread.thread(fakeAnnotationUI.state);
       assert.calledWith(fakeBuildThread, sinon.match([annotation]));
     });
 
@@ -112,7 +95,7 @@ describe('rootThread', function () {
       fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, {
         selectedAnnotationMap: {id1: true, id2: true},
       });
-      rootThread.rebuild();
+      rootThread.thread(fakeAnnotationUI.state);
       assert.calledWith(fakeBuildThread, [], sinon.match({
         selected: ['id1', 'id2'],
       }));
@@ -122,7 +105,7 @@ describe('rootThread', function () {
       fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, {
         expanded: {id1: true, id2: true},
       });
-      rootThread.rebuild();
+      rootThread.thread(fakeAnnotationUI.state);
       assert.calledWith(fakeBuildThread, [], sinon.match({
         expanded: {id1: true, id2: true},
       }));
@@ -132,22 +115,13 @@ describe('rootThread', function () {
       fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, {
         forceVisible: {id1: true, id2: true},
       });
-      rootThread.rebuild();
+      rootThread.thread(fakeAnnotationUI.state);
       assert.calledWith(fakeBuildThread, [], sinon.match({
         forceVisible: ['id1', 'id2'],
       }));
     });
   });
 
-  context('when the annotationUI state changes', function () {
-    it('rebuilds the root thread', function () {
-      assertRebuildsThread(function () {
-        var subscriber = fakeAnnotationUI.subscribe.args[0][0];
-        subscriber();
-      });
-    });
-  });
-
   describe('when the sort order changes', function () {
     function sortBy(annotations, sortCompareFn) {
       return annotations.slice().sort(function (a,b) {
@@ -181,7 +155,7 @@ describe('rootThread', function () {
         sortKey: testCase.order,
         sortKeysAvailable: [testCase.order],
       });
-      rootThread.rebuild();
+      rootThread.thread(fakeAnnotationUI.state);
       var sortCompareFn = fakeBuildThread.args[0][1].sortCompareFn;
       var actualOrder = sortBy(annotations, sortCompareFn).map(function (annot) {
         return annotations.indexOf(annot);
@@ -202,7 +176,7 @@ describe('rootThread', function () {
       fakeSearchFilter.generateFacetedFilter.returns(filters);
       fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state,
         {filterQuery: 'queryterm'});
-      rootThread.rebuild();
+      rootThread.thread(fakeAnnotationUI.state);
       var filterFn = fakeBuildThread.args[0][1].filterFn;
 
       fakeViewFilter.filter.returns([annotation]);

h/static/scripts/test/stream-controller-test.coffee

@@ -43,6 +43,7 @@ describe 'StreamController', ->
       setCollapsed: sandbox.spy()
       setForceVisible: sandbox.spy()
       setSortKey: sandbox.spy()
+      subscribe: sandbox.spy()
     }
 
     fakeParams = {id: 'test'}

h/static/scripts/virtual-thread-list.js

@@ -63,6 +63,9 @@ VirtualThreadList.prototype.detach = function () {
  * matching annotations changes.
  */
 VirtualThreadList.prototype.setRootThread = function (thread) {
+  if (thread === this._rootThread) {
+    return;
+  }
   this._rootThread = thread;
   this._updateVisibleThreads();
 };

h/static/scripts/widget-controller.js

@@ -61,11 +61,19 @@ module.exports = function WidgetController(
     return elementHeight + marginHeight;
   }
 
+  function thread() {
+    return rootThread.thread(annotationUI.getState());
+  }
+
   // `visibleThreads` keeps track of the subset of all threads matching the
   // current filters which are in or near the viewport and the view then renders
   // only those threads, using placeholders above and below the visible threads
   // to reserve space for threads which are not actually rendered.
-  var visibleThreads = new VirtualThreadList($scope, window, rootThread.thread());
+  var visibleThreads = new VirtualThreadList($scope, window, thread());
+  annotationUI.subscribe(function () {
+    visibleThreads.setRootThread(thread());
+  });
+
   visibleThreads.on('changed', function (state) {
     $scope.virtualThreadList = {
       visibleThreads: state.visibleThreads,
@@ -83,9 +91,7 @@ module.exports = function WidgetController(
       });
     }, 50);
   });
-  rootThread.on('changed', function (thread) {
-    visibleThreads.setRootThread(thread);
-  });
+
   $scope.$on('$destroy', function () {
     visibleThreads.detach();
   });
@@ -264,10 +270,6 @@ module.exports = function WidgetController(
     annotationUI.setFilterQuery(query);
   });
 
-  $scope.rootThread = function () {
-    return rootThread.thread();
-  };
-
   $scope.setCollapsed = function (id, collapsed) {
     annotationUI.setCollapsed(id, collapsed);
   };
@@ -334,11 +336,11 @@ module.exports = function WidgetController(
   });
 
   $scope.visibleCount = function () {
-    return visibleCount(rootThread.thread());
+    return visibleCount(thread());
   };
 
   $scope.topLevelThreadCount = function () {
-    return rootThread.thread().totalChildren;
+    return thread().totalChildren;
   };
 
   /**