fix(scopes): correctly meassure perf impact of one-time expressions

src/modules/scopes.js

@@ -79,37 +79,20 @@ function decorateRootScope($delegate, $parse) {
   var _digestEvents = [];
   var skipNextPerfWatchers = false;
   scopePrototype.$watch = function (watchExpression, reactionFunction) {
-    // if `skipNextPerfWatchers` is true, this means the previous run of the
-    // `$watch` decorator was a one time binding expression and this invocation
-    // of the $watch function has the `oneTimeInterceptedExpression` (internal angular function)
-    // as the `watchExpression` parameter. If we decorate it with the performance
-    // timers function this will cause us to invoke `oneTimeInterceptedExpression`
-    // on subsequent digest loops and will update the one time bindings
-    // if anything mutated the property.
-    if (skipNextPerfWatchers) {
-      skipNextPerfWatchers = false;
-      return _watch.apply(this, arguments);
-    }
-
-    if (typeof watchExpression === 'string' &&
-        isOneTimeBindExp(watchExpression)) {
-      skipNextPerfWatchers = true;
-      return _watch.apply(this, arguments);
-    }
-    var watchStr = humanReadableWatchExpression(watchExpression);
-    var scopeId = this.$id;
-    var expressions = null;
-    if (typeof watchExpression === 'function') {
-      expressions = watchExpression.expressions;
-      if (Object.prototype.toString.call(expressions) === '[object Array]' &&
-          expressions.some(isOneTimeBindExp)) {
-        skipNextPerfWatchers = true;
-        return _watch.apply(this, arguments);
-      }
-
-      arguments[0] = function () {
+    // Convert the `watchExpression` to a function (if not already one).
+    // This is also the first thing `Scope.$watch()` does.
+    var parsedExpression = $parse(watchExpression);
+
+    // Only intercept this call if there is no `$$watchDelegate`.
+    // (With `$$watchDelegate` there will be subsequent calls to `$watch` (if necessary)).
+    if (!parsedExpression.$$watchDelegate) {
+      var scopeId = this.$id;
+      var watchStr = humanReadableWatchExpression(watchExpression);
+
+      // Intercept the `watchExpression` (if any).
+      arguments[0] = simpleExtend(function() {
         var start = perf.now();
-        var ret = watchExpression.apply(this, arguments);
+        var ret = parsedExpression.apply(this, arguments);
         var end = perf.now();
         _digestEvents.push({
           eventType: 'scope:watch',
@@ -118,36 +101,23 @@ function decorateRootScope($delegate, $parse) {
           time: end - start
         });
         return ret;
-      };
-    } else {
-      var thatScope = this;
-      arguments[0] = function () {
-        var start = perf.now();
-        var ret = thatScope.$eval(watchExpression);
-        var end = perf.now();
-        _digestEvents.push({
-          eventType: 'scope:watch',
-          id: scopeId,
-          watch: watchStr,
-          time: end - start
-        });
-        return ret;
-      };
-    }
-
-    if (typeof reactionFunction === 'function') {
-      arguments[1] = function () {
-        var start = perf.now();
-        var ret = reactionFunction.apply(this, arguments);
-        var end = perf.now();
-        _digestEvents.push({
-          eventType: 'scope:reaction',
-          id: scopeId,
-          watch: watchStr,
-          time: end - start
-        });
-        return ret;
-      };
+      }, parsedExpression);
+
+      // Intercept the `reactionFunction` (if any).
+      if (typeof reactionFunction === 'function') {
+        arguments[1] = function() {
+          var start = perf.now();
+          var ret = reactionFunction.apply(this, arguments);
+          var end = perf.now();
+          _digestEvents.push({
+            eventType: 'scope:reaction',
+            id: scopeId,
+            watch: watchStr,
+            time: end - start
+          });
+          return ret;
+        };
+      }
     }
 
     return _watch.apply(this, arguments);
@@ -355,12 +325,13 @@ function humanReadableWatchExpression (fn) {
   return fn.toString();
 }
 
-function isOneTimeBindExp(exp) {
-  // this is the same code angular 1.3.15 has to check
-  // for a one time bind expression
-  return exp.charAt(0) === ':' && exp.charAt(1) === ':';
-}
-
 function convertIdToOriginalType(scopeId) {
   return (angular.version.minor < 3) ? scopeId : parseInt(scopeId, 10);
 }
+
+function simpleExtend(dst, src) {
+  Object.keys(src).forEach(function(key) {
+    dst[key] = src[key];
+  });
+  return dst;
+}

test/scopes.spec.js

@@ -72,33 +72,88 @@ describe('ngHintScopes', function() {
     });
 
     if (angular.version.minor >= 3) {
-      it('should not run perf timers for one time bind expressions passed to watch', function() {
-        var calls = hint.emit.calls;
-        scope.$watch('::a.b', function() {});
-        expect(calls.count()).toBe(0);
-
-        scope.$apply();
-        var evt = calls.mostRecent().args[1].events[0];
-        expect(calls.count()).toBe(1);
-        expect(evt).toBeUndefined();
-      });
-
-      it('should not run perf timers for one time template bindings', function() {
-        var elt = angular.element(
-          '<div>' +
-          '<span>{{::a}}</span>' +
-          '<button ng-click="a = \'bar\'">Set</button>' +
-          '</div>'
-        );
-        scope.a = 'foo';
-        var view = $compile(elt)(scope);
-        scope.$apply();
-        var $binding = view.find('span');
-        var $button = view.find('button');
-
-        $button.triggerHandler('click');
-        scope.$apply();
-        expect($binding.text()).toBe('foo');
+      describe('one-time expressions', function() {
+        // Helpers
+        function countWatchEventsFor(callIdx) {
+          var args = hint.emit.calls.argsFor(callIdx);
+          var events = args[1].events;
+          var watchEvents = events.filter(function(evt) {
+            return evt.eventType === 'scope:watch';
+          });
+
+          return watchEvents.length;
+        }
+
+        it('should correctly trigger perf timers when passed to `$watch`', function() {
+          var calls = hint.emit.calls;
+
+          var reactions = [0, 0, 0];
+          var exps = [
+            '::c.d',
+            '  ::c.d  ',
+            '  ::  c.d  '
+          ].forEach(function(exp, idx) {
+            scope.$watch(exp, function(v) { reactions[idx]++; });
+          });
+
+          expect(calls.count()).toBe(0);
+
+          scope.$apply();
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(6);   // First $digest: 2 loops
+          expect(reactions).toEqual([1, 1, 1]);
+
+          calls.reset();
+          scope.$apply();
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(3);    // No change: 1 loop
+          expect(reactions).toEqual([1, 1, 1]);
+
+          calls.reset();
+          scope.$apply('c.d = "foo"');
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(6);   // Change: 2 loops
+          expect(reactions).toEqual([2, 2, 2]);
+
+          calls.reset();
+          scope.$apply('c.d = "bar"');
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(0);   // Already settled: 0 loops
+          expect(reactions).toEqual([2, 2, 2]);
+        });
+
+        it('should correctly trigger perf timers when used in template bindings', function() {
+          var calls = hint.emit.calls;
+
+          $compile(
+            '<div>' +
+              '<span>{{::c.d}}</span>' +
+              '<span>{{  ::c.d  }}</span>' +
+              '<span>{{  ::  c.d  }}</span>' +
+              '<span>{{::c.d}}{{  ::c.d  }}{{  ::  c.d  }}</span>' +
+            '</div>'
+          )(scope);
+
+          calls.reset();
+          scope.$apply();
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(12);   // First $digest: 2 loops
+
+          calls.reset();
+          scope.$apply();
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(6);    // No change: 1 loop
+
+          calls.reset();
+          scope.$apply('c.d = "foo"');
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(12);   // Change: 2 loops
+
+          calls.reset();
+          scope.$apply('c.d = "bar"');
+          expect(calls.count()).toBe(1);
+          expect(countWatchEventsFor(0)).toBe(0);    // Already settled: 0 loops
+        });
       });
     }