From b3b514e61600aa155391957f61e89a1695c0d4e3 Mon Sep 17 00:00:00 2001
From: Jason Bedard <jason.bedard@allocadia.com>
Date: Fri, 4 May 2018 16:21:08 -0700
Subject: [PATCH] perf($compile): remove use of deepEquals and $stateful
 interceptor from bidi bindings

Previously bidirectional bindings were implemented using a custom watch
interceptor which stored the component/directive inner value and therefore
had to be marked as `$stateful`. The use of a `$stateful` interceptor
prevented some optimizations from being used.

As a result of using a `$stateful` interceptor the bindings would be
executed on each and every digest. For literal expressions this would
recreate the literal every digest and therefore require `objectEquality
= true`, causing a `angular.equals` call on every digest.
---
 src/ng/compile.js      |  31 ++++----
 test/ng/compileSpec.js | 170 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 174 insertions(+), 27 deletions(-)

diff --git a/src/ng/compile.js b/src/ng/compile.js
index ea25e0c2e672..68286157aba6 100644
--- a/src/ng/compile.js
+++ b/src/ng/compile.js
@@ -309,11 +309,9 @@
  *   to the local value, since it will be impossible to sync them back to the parent scope.
  *
  *   By default, the {@link ng.$rootScope.Scope#$watch `$watch`}
- *   method is used for tracking changes, and the equality check is based on object identity.
- *   However, if an object literal or an array literal is passed as the binding expression, the
- *   equality check is done by value (using the {@link angular.equals} function). It's also possible
- *   to watch the evaluated value shallowly with {@link ng.$rootScope.Scope#$watchCollection
- *   `$watchCollection`}: use `=*` or `=*attr`
+ *   method is used for tracking changes between the local scope property and the expression passed
+ *   via the attribute. It's also possible to watch the evaluated value shallowly with
+ *   {@link ng.$rootScope.Scope#$watchCollection `$watchCollection`}: use `=*` or `=*attr`.
  *
   * * `<` or `<attr` - set up a one-way (one-directional) binding between a local scope property and an
  *   expression passed via the attribute `attr`. The expression is evaluated in the context of the
@@ -3535,7 +3533,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
         optional = definition.optional,
         mode = definition.mode, // @, =, <, or &
         lastValue,
-        parentGet, parentSet, compare, removeWatch;
+        parentGet, parentSet, removeWatch;
 
         switch (mode) {
 
@@ -3576,11 +3574,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
             if (optional && !attrs[attrName]) break;
 
             parentGet = $parse(attrs[attrName]);
-            if (parentGet.literal) {
-              compare = equals;
-            } else {
-              compare = simpleCompare;
-            }
             parentSet = parentGet.assign || function() {
               // reset the change, or we will throw this exception on every $digest
               lastValue = destination[scopeName] = parentGet(scope);
@@ -3588,11 +3581,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                   'Expression \'{0}\' in attribute \'{1}\' used with directive \'{2}\' is non-assignable!',
                   attrs[attrName], attrName, directive.name);
             };
+            var childGet = function childGet() { return destination[scopeName]; };
+
             lastValue = destination[scopeName] = parentGet(scope);
-            var parentValueWatch = function parentValueWatch(parentValue) {
-              if (!compare(parentValue, destination[scopeName])) {
+
+            var bidiWatchAction = function bidiWatchAction(newValues) {
+              var parentValue = newValues[0];
+              if (!simpleCompare(parentValue, destination[scopeName])) {
                 // we are out of sync and need to copy
-                if (!compare(parentValue, lastValue)) {
+                if (!simpleCompare(parentValue, lastValue)) {
                   // parent changed and it has precedence
                   destination[scopeName] = parentValue;
                 } else {
@@ -3601,13 +3598,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                 }
               }
               lastValue = parentValue;
-              return lastValue;
             };
-            parentValueWatch.$stateful = true;
             if (definition.collection) {
-              removeWatch = scope.$watchCollection(attrs[attrName], parentValueWatch);
+              removeWatch = scope.$watchCollection(parentGet, function bidiCollectionWatchAction(parentValue) { destination[scopeName] = parentValue; });
             } else {
-              removeWatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal);
+              removeWatch = scope.$watchGroup([parentGet, childGet], bidiWatchAction);
             }
             removeWatchCollection.push(removeWatch);
             break;
diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js
index ed4b2f97cfc1..bd87882935bf 100644
--- a/test/ng/compileSpec.js
+++ b/test/ng/compileSpec.js
@@ -5416,21 +5416,21 @@ describe('$compile', function() {
 
         inject(function($rootScope) {
           compile('<div other-tpl-dir param1="::foo" param2="bar"></div>');
-          expect(countWatches($rootScope)).toEqual(6); // 4 -> template watch group, 2 -> '='
+          expect(countWatches($rootScope)).toEqual(8); // 4 -> template watch group, 4 -> '='
           $rootScope.$digest();
           expect(element.html()).toBe('1:;2:;3:;4:');
-          expect(countWatches($rootScope)).toEqual(6);
+          expect(countWatches($rootScope)).toEqual(8);
 
           $rootScope.foo = 'foo';
           $rootScope.$digest();
           expect(element.html()).toBe('1:foo;2:;3:foo;4:');
-          expect(countWatches($rootScope)).toEqual(4);
+          expect(countWatches($rootScope)).toEqual(6);
 
           $rootScope.foo = 'baz';
           $rootScope.bar = 'bar';
           $rootScope.$digest();
           expect(element.html()).toBe('1:foo;2:bar;3:foo;4:bar');
-          expect(countWatches($rootScope)).toEqual(3);
+          expect(countWatches($rootScope)).toEqual(5);
 
           $rootScope.bar = 'baz';
           $rootScope.$digest();
@@ -5487,18 +5487,18 @@ describe('$compile', function() {
           compile('<div other-tpl-dir param1="::foo" param2="bar"></div>');
           $rootScope.$digest();
           expect(element.html()).toBe('1:;2:;3:;4:');
-          expect(countWatches($rootScope)).toEqual(6); // 4 -> template watch group, 2 -> '='
+          expect(countWatches($rootScope)).toEqual(8); // 4 -> template watch group, 4 -> '='
 
           $rootScope.foo = 'foo';
           $rootScope.$digest();
           expect(element.html()).toBe('1:foo;2:;3:foo;4:');
-          expect(countWatches($rootScope)).toEqual(4);
+          expect(countWatches($rootScope)).toEqual(6);
 
           $rootScope.foo = 'baz';
           $rootScope.bar = 'bar';
           $rootScope.$digest();
           expect(element.html()).toBe('1:foo;2:bar;3:foo;4:bar');
-          expect(countWatches($rootScope)).toEqual(3);
+          expect(countWatches($rootScope)).toEqual(5);
 
           $rootScope.bar = 'baz';
           $rootScope.$digest();
@@ -5733,6 +5733,37 @@ describe('$compile', function() {
           expect(componentScope.reference).toEqual({name: 'b'});
         }));
 
+        it('should copy parent changes, even if deep equal to old', inject(function() {
+          compile('<div><span my-component reference="{person: person}">');
+
+          $rootScope.person = {name: 'a'};
+          $rootScope.$apply();
+          expect(componentScope.reference).toEqual({person: {name: 'a'}});
+
+          var instance = componentScope.reference;
+          $rootScope.person = {name: 'a'};
+          $rootScope.$apply();
+          expect(componentScope.reference).not.toBe(instance);
+          expect(componentScope.reference).toEqual(instance);
+        }));
+
+        it('should not copy on parent changes to nested objects', inject(function() {
+          compile('<div><span my-component reference="{subObj: subObj}">');
+
+          var subObj = {foo: 42};
+
+          $rootScope.subObj = subObj;
+          $rootScope.$apply();
+          expect(componentScope.reference).toEqual({subObj: subObj});
+
+          var firstVal = componentScope.reference;
+
+          $rootScope.subObj.foo = true;
+          $rootScope.$apply();
+          expect(componentScope.reference).toBe(firstVal);
+          expect(componentScope.reference).toEqual({subObj: subObj});
+        }));
+
         it('should not change the component when parent does not change', inject(function() {
           compile('<div><span my-component reference="{name: name}">');
 
@@ -5771,6 +5802,46 @@ describe('$compile', function() {
           }
         }));
 
+        it('should work with filtered literal objects within array literals', inject(function() {
+          $rootScope.gabName = 'Gabriel';
+          $rootScope.tonyName = 'Tony';
+          $rootScope.query = '';
+          $rootScope.$apply();
+
+          compile('<div><span my-component reference="[{name: gabName}, {name: tonyName}] | filter:query">');
+
+          expect(componentScope.reference).toEqual([{name: $rootScope.gabName}, {name: $rootScope.tonyName}]);
+
+          $rootScope.query = 'Gab';
+          $rootScope.$apply();
+
+          expect(componentScope.reference).toEqual([{name: $rootScope.gabName}]);
+
+          $rootScope.tonyName = 'Gab';
+          $rootScope.$apply();
+
+          expect(componentScope.reference).toEqual([{name: $rootScope.gabName}, {name: $rootScope.tonyName}]);
+        }));
+
+        it('should work with filtered constant literal objects within array literals (constant)', inject(function() {
+          $rootScope.query = '';
+          $rootScope.$apply();
+
+          compile('<div><span my-component reference="[{name: \'Gabriel\'}, {name: \'Toni\'}] | filter:query">');
+
+          expect(componentScope.reference).toEqual([{name: 'Gabriel'}, {name: 'Toni'}]);
+
+          $rootScope.query = 'Gab';
+          $rootScope.$apply();
+
+          expect(componentScope.reference).toEqual([{name: 'Gabriel'}]);
+
+          $rootScope.query = 'i';
+          $rootScope.$apply();
+
+          expect(componentScope.reference).toEqual([{name: 'Gabriel'}, {name: 'Toni'}]);
+        }));
+
       });
 
     });
@@ -5829,8 +5900,48 @@ describe('$compile', function() {
         $rootScope.$apply();
 
         expect(componentScope.colref).toEqual([$rootScope.collection[0]]);
-        expect(componentScope.colrefAlias).toEqual([$rootScope.collection[0]]);
-        expect(componentScope.$colrefAlias).toEqual([$rootScope.collection[0]]);
+        expect(componentScope.colrefAlias).toEqual(componentScope.colref);
+        expect(componentScope.$colrefAlias).toEqual(componentScope.colref);
+
+        $rootScope.collection[1].name = 'Gab';
+        $rootScope.$apply();
+
+        expect(componentScope.colref).toEqual($rootScope.collection);
+        expect(componentScope.colrefAlias).toEqual(componentScope.colref);
+        expect(componentScope.$colrefAlias).toEqual(componentScope.colref);
+      }));
+
+      it('should work with filtered objects within a literal collection', inject(function() {
+        $rootScope.gab = {
+          name: 'Gabriel',
+          value: 18
+        };
+        $rootScope.tony = {
+          name: 'Tony',
+          value: 91
+        };
+        $rootScope.query = '';
+        $rootScope.$apply();
+
+        compile('<div><span my-component colref="[gab, tony] | filter:query" $colref$="[gab, tony] | filter:query">');
+
+        expect(componentScope.colref).toEqual([$rootScope.gab, $rootScope.tony]);
+        expect(componentScope.colrefAlias).toEqual(componentScope.colref);
+        expect(componentScope.$colrefAlias).toEqual(componentScope.colref);
+
+        $rootScope.query = 'Gab';
+        $rootScope.$apply();
+
+        expect(componentScope.colref).toEqual([$rootScope.gab]);
+        expect(componentScope.colrefAlias).toEqual(componentScope.colref);
+        expect(componentScope.$colrefAlias).toEqual(componentScope.colref);
+
+        $rootScope.tony.name = 'Gab';
+        $rootScope.$apply();
+
+        expect(componentScope.colref).toEqual([$rootScope.gab, $rootScope.tony]);
+        expect(componentScope.colrefAlias).toEqual(componentScope.colref);
+        expect(componentScope.$colrefAlias).toEqual(componentScope.colref);
       }));
 
       it('should update origin scope when isolate scope changes', inject(function() {
@@ -6187,6 +6298,47 @@ describe('$compile', function() {
       }));
 
 
+      it('should work with filtered literal objects within array literals', inject(function() {
+        $rootScope.gabName = 'Gabriel';
+        $rootScope.tonyName = 'Tony';
+        $rootScope.query = '';
+        $rootScope.$apply();
+
+        compile('<div><span my-component ow-ref="[{name: gabName}, {name: tonyName}] | filter:query">');
+
+        expect(componentScope.owRef).toEqual([{name: $rootScope.gabName}, {name: $rootScope.tonyName}]);
+
+        $rootScope.query = 'Gab';
+        $rootScope.$apply();
+
+        expect(componentScope.owRef).toEqual([{name: $rootScope.gabName}]);
+
+        $rootScope.tonyName = 'Gab';
+        $rootScope.$apply();
+
+        expect(componentScope.owRef).toEqual([{name: $rootScope.gabName}, {name: $rootScope.tonyName}]);
+      }));
+
+      it('should work with filtered constant literal objects within array literals', inject(function() {
+        $rootScope.query = '';
+        $rootScope.$apply();
+
+        compile('<div><span my-component ow-ref="[{name: \'Gabriel\'}, {name: \'Toni\'}] | filter:query">');
+
+        expect(componentScope.owRef).toEqual([{name: 'Gabriel'}, {name: 'Toni'}]);
+
+        $rootScope.query = 'Gab';
+        $rootScope.$apply();
+
+        expect(componentScope.owRef).toEqual([{name: 'Gabriel'}]);
+
+        $rootScope.query = 'i';
+        $rootScope.$apply();
+
+        expect(componentScope.owRef).toEqual([{name: 'Gabriel'}, {name: 'Toni'}]);
+      }));
+
+
       describe('literal objects', function() {
         it('should copy parent changes', inject(function() {
           compile('<div><span my-component ow-ref="{name: name}">');