Merge pull request #3685 from spalger/fix/multiWatchRaceCondition

[$multiWatch] simplified the code and fixed a race condition

src/kibana/components/watch_multi.js

@@ -31,83 +31,77 @@ define(function (require) {
        *
        * @param  {array[string|function|obj]} expressions - the list of expressions to $watch
        * @param  {Function} fn - the callback function
-       * @param  {boolean} deep - should the watchers be created as deep watchers?
-       * @return {undefined}
+       * @return {Function} - an unwatch function, just like the return value of $watch
        */
-      $delegate.constructor.prototype.$watchMulti = function (expressions, fn, deep) {
+      $delegate.constructor.prototype.$watchMulti = function (expressions, fn) {
         if (!_.isArray(expressions)) throw new TypeError('expected an array of expressions to watch');
         if (!_.isFunction(fn)) throw new TypeError('expected a function that is triggered on each watch');
 
         var $scope = this;
-        var fired = false;
-        var queue = [];
         var vals = new Array(expressions.length);
         var prev = new Array(expressions.length);
+        var fire = false;
 
-        function normalizeExpression(expr) {
+        // first, register all of the multi-watchers
+        var unwatchers = expressions.map(function (expr, i) {
+          expr = normalizeExpression($scope, expr);
           if (!expr) return;
 
-          var norm = {
-            fn: $scope.$watch,
-            deep: false
-          };
-
-          if (_.isFunction(expr)) return _.assign(norm, { get: expr });
-          if (_.isObject(expr)) return _.assign(norm, expr);
-          if (!_.isString(expr)) return;
-
-          if (expr.substr(0, 2) === '[]') {
-            return _.assign(norm, {
-              fn: $scope.$watchCollection,
-              get: expr.substr(2)
-            });
-          }
-
-          if (expr.charAt(0) === '=') {
-            return _.assign(norm, {
-              deep: true,
-              get: expr.substr(1)
-            });
-          }
-
-          return _.assign(norm, { get: expr });
-        }
-
-        expressions.forEach(function (expr, i) {
-          expr = normalizeExpression(expr);
-          if (!expr) return;
-
-          queue.push(expr);
-          expr.fn.call($scope, expr.get, function (newVal, oldVal) {
+          return expr.fn.call($scope, expr.get, function (newVal, oldVal) {
             vals[i] = newVal;
-
-            if (queue) {
-              prev[i] = oldVal;
-              _.pull(queue, expr);
-              if (queue.length > 0) return;
-              queue = false;
-            }
-
-            if (fired) return;
-            fired = true;
-            $scope.$evalAsync(function () {
-              fired = false;
-
-              if (fn.length) {
-                fn(vals.slice(0), prev.slice(0));
-              } else {
-                fn();
-              }
-
-              for (var i = 0; i < vals.length; i++) {
-                prev[i] = vals[i];
-              }
-            });
+            prev[i] = oldVal;
+            fire = true;
           }, expr.deep);
         });
+
+        // then, the watcher that checks to see if any of
+        // the other watchers triggered this cycle
+        var flip = false;
+        unwatchers.push($scope.$watch(function () {
+          if (fire) {
+            fire = false;
+            flip = !flip;
+          }
+          return flip;
+        }, function () {
+          fn(vals.slice(0), prev.slice(0));
+          vals.forEach(function (v, i) {
+            prev[i] = v;
+          });
+        }));
+
+        return _.partial(_.callEach, unwatchers);
       };
 
+      function normalizeExpression($scope, expr) {
+        if (!expr) return;
+        var norm = {
+          fn: $scope.$watch,
+          deep: false
+        };
+
+        if (_.isFunction(expr)) return _.assign(norm, { get: expr });
+        if (_.isObject(expr)) return _.assign(norm, expr);
+        if (!_.isString(expr)) return;
+
+        if (expr.substr(0, 2) === '[]') {
+          return _.assign(norm, {
+            fn: $scope.$watchCollection,
+            get: expr.substr(2)
+          });
+        }
+
+        if (expr.charAt(0) === '=') {
+          return _.assign(norm, {
+            deep: true,
+            get: expr.substr(1)
+          });
+        }
+
+        return _.assign(norm, { get: expr });
+      }
+
       return $delegate;
     });
   });
-});
+});

test/unit/specs/components/watch_multi.js

@@ -82,23 +82,48 @@ define(function (require) {
       ]);
     });
 
-    it('does not pass args unless the function will use them', function () {
-      var calls = 0;
+    it('the current value is always up to date', function () {
+      var count = 0;
 
-      $scope.one = 'a';
-      $scope.two = 'b';
-      $scope.three = 'c';
-      $scope.$watchMulti([
-        'one',
-        'two',
-        'three'
-      ], function () {
-        calls++;
-        expect(arguments).to.have.length(0);
+      $scope.vals = [1, 0];
+      $scope.$watchMulti([ 'vals[0]', 'vals[1]' ], function (cur, prev) {
+        expect(cur).to.eql($scope.vals);
+        count++;
       });
-      $rootScope.$apply();
 
-      expect(calls).to.be(1);
+      var $child = $scope.$new();
+      $child.$watch('vals[0]', function (cur) {
+        $child.vals[1] = cur;
+      });
+
+      $rootScope.$apply();
+      expect(count).to.be(2);
+    });
+
+    it('returns a working unwatch function', function () {
+      $scope.a = 0;
+      $scope.b = 0;
+      var triggers = 0;
+      var unwatch = $scope.$watchMulti(['a', 'b'], function () { triggers++; });
+
+      // initial watch
+      $scope.$apply();
+      expect(triggers).to.be(1);
+
+      // prove that it triggers on chagne
+      $scope.a++;
+      $scope.$apply();
+      expect(triggers).to.be(2);
+
+      // remove watchers
+      expect($scope.$$watchers).to.not.eql([]);
+      unwatch();
+      expect($scope.$$watchers).to.eql([]);
+
+      // prove that it doesn't trigger anymore
+      $scope.a++;
+      $scope.$apply();
+      expect(triggers).to.be(2);
     });
   });
-});
+});