fix(Scope): $watchCollection should call listener with oldValue

Originally we destroyed the oldValue by incrementaly copying over portions of the newValue
into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue
by the time we called the watchCollection listener.

The fix creates a copy of the newValue each time a change is detected and then uses that
copy *the next time* a change is detected.

To make `$watchCollection` behave the same way as `$watch`, during the first iteration
the listener is called with newValue and oldValue being identical.

Since many of the corner-cases are already covered by existing tests, I refactored the
test logging to include oldValue and made the tests more readable.

Closes #2621
Closes #5661
Closes #5688
Closes #6736

Author: IgorMinar

src/ng/rootScope.js

@@ -398,30 +398,40 @@ function $RootScopeProvider(){
        *    {@link ng.$rootScope.Scope#$digest $digest} cycle. Any shallow change within the
        *    collection will trigger a call to the `listener`.
        *
-       * @param {function(newCollection, oldCollection, scope)} listener a callback function that is
-       *    fired with both the `newCollection` and `oldCollection` as parameters.
-       *    The `newCollection` object is the newly modified data obtained from the `obj` expression
-       *    and the `oldCollection` object is a copy of the former collection data.
-       *    The `scope` refers to the current scope.
+       * @param {function(newCollection, oldCollection, scope)} listener a callback function called
+       *    when a change is detected.
+       *    - The `newCollection` object is the newly modified data obtained from the `obj` expression
+       *    - The `oldCollection` object is a copy of the former collection data.
+       *      Due to performance considerations, the`oldCollection` value is computed only if the
+       *      `listener` function declares two or more arguments.
+       *    - The `scope` argument refers to the current scope.
        *
        * @returns {function()} Returns a de-registration function for this listener. When the
        *    de-registration function is executed, the internal watch operation is terminated.
        */
       $watchCollection: function(obj, listener) {
         var self = this;
-        var oldValue;
+        // the current value, updated on each dirty-check run
         var newValue;
+        // a shallow copy of the newValue from the last dirty-check run,
+        // updated to match newValue during dirty-check run
+        var oldValue;
+        // a shallow copy of the newValue from when the last change happened
+        var veryOldValue;
+        // only track veryOldValue if the listener is asking for it
+        var trackVeryOldValue = (listener.length > 1);
         var changeDetected = 0;
         var objGetter = $parse(obj);
         var internalArray = [];
         var internalObject = {};
+        var initRun = true;
         var oldLength = 0;
 
         function $watchCollectionWatch() {
           newValue = objGetter(self);
           var newLength, key;
 
-          if (!isObject(newValue)) {
+          if (!isObject(newValue)) { // if primitive
             if (oldValue !== newValue) {
               oldValue = newValue;
               changeDetected++;
@@ -487,7 +497,32 @@ function $RootScopeProvider(){
         }
 
         function $watchCollectionAction() {
-          listener(newValue, oldValue, self);
+          if (initRun) {
+            initRun = false;
+            listener(newValue, newValue, self);
+          } else {
+            listener(newValue, veryOldValue, self);
+          }
+
+          // make a copy for the next time a collection is changed
+          if (trackVeryOldValue) {
+            if (!isObject(newValue)) {
+              //primitive
+              veryOldValue = newValue;
+            } else if (isArrayLike(newValue)) {
+              veryOldValue = new Array(newValue.length);
+              for (var i = 0; i < newValue.length; i++) {
+                veryOldValue[i] = newValue[i];
+              }
+            } else { // if object
+              veryOldValue = {};
+              for (var key in newValue) {
+                if (hasOwnProperty.call(newValue, key)) {
+                  veryOldValue[key] = newValue[key];
+                }
+              }
+            }
+          }
         }
 
         return this.$watch($watchCollectionWatch, $watchCollectionAction);

test/ng/rootScopeSpec.js

@@ -483,104 +483,127 @@ describe('Scope', function() {
     describe('$watchCollection', function() {
       var log, $rootScope, deregister;
 
-      beforeEach(inject(function(_$rootScope_) {
-        log = [];
+      beforeEach(inject(function(_$rootScope_, _log_) {
         $rootScope = _$rootScope_;
-        deregister = $rootScope.$watchCollection('obj', function logger(obj) {
-          log.push(toJson(obj));
+        log = _log_;
+        deregister = $rootScope.$watchCollection('obj', function logger(newVal, oldVal) {
+          var msg = {newVal: newVal, oldVal: oldVal};
+
+          if (newVal === oldVal) {
+            msg.identical = true;
+          }
+
+          log(msg);
         });
       }));
 
 
       it('should not trigger if nothing change', inject(function($rootScope) {
         $rootScope.$digest();
-        expect(log).toEqual([undefined]);
+        expect(log).toEqual([{ newVal : undefined, oldVal : undefined, identical : true }]);
+        log.reset();
 
         $rootScope.$digest();
-        expect(log).toEqual([undefined]);
+        expect(log).toEqual([]);
       }));
 
 
-      it('should allow deregistration', inject(function($rootScope) {
+      it('should allow deregistration', function() {
         $rootScope.obj = [];
         $rootScope.$digest();
-
-        expect(log).toEqual(['[]']);
+        expect(log.toArray().length).toBe(1);
+        log.reset();
 
         $rootScope.obj.push('a');
         deregister();
 
         $rootScope.$digest();
-        expect(log).toEqual(['[]']);
-      }));
+        expect(log).toEqual([]);
+      });
 
 
       describe('array', function() {
+
+        it('should return oldCollection === newCollection only on the first listener call',
+            inject(function($rootScope, log) {
+
+          // first time should be identical
+          $rootScope.obj = ['a', 'b'];
+          $rootScope.$digest();
+          expect(log).toEqual([{newVal: ['a', 'b'], oldVal: ['a', 'b'], identical: true}]);
+          log.reset();
+
+          // second time should be different
+          $rootScope.obj[1] = 'c';
+          $rootScope.$digest();
+          expect(log).toEqual([{newVal: ['a', 'c'], oldVal: ['a', 'b']}]);
+        }));
+
+
         it('should trigger when property changes into array', function() {
           $rootScope.obj = 'test';
           $rootScope.$digest();
-          expect(log).toEqual(['"test"']);
+          expect(log.empty()).toEqual([{newVal: "test", oldVal: "test", identical: true}]);
 
           $rootScope.obj = [];
           $rootScope.$digest();
-          expect(log).toEqual(['"test"', '[]']);
+          expect(log.empty()).toEqual([{newVal: [], oldVal: "test"}]);
 
           $rootScope.obj = {};
           $rootScope.$digest();
-          expect(log).toEqual(['"test"', '[]', '{}']);
+          expect(log.empty()).toEqual([{newVal: {}, oldVal: []}]);
 
           $rootScope.obj = [];
           $rootScope.$digest();
-          expect(log).toEqual(['"test"', '[]', '{}', '[]']);
+          expect(log.empty()).toEqual([{newVal: [], oldVal: {}}]);
 
           $rootScope.obj = undefined;
           $rootScope.$digest();
-          expect(log).toEqual(['"test"', '[]', '{}', '[]', undefined]);
+          expect(log.empty()).toEqual([{newVal: undefined, oldVal: []}]);
         });
 
 
         it('should not trigger change when object in collection changes', function() {
           $rootScope.obj = [{}];
           $rootScope.$digest();
-          expect(log).toEqual(['[{}]']);
+          expect(log.empty()).toEqual([{newVal: [{}], oldVal: [{}], identical: true}]);
 
           $rootScope.obj[0].name = 'foo';
           $rootScope.$digest();
-          expect(log).toEqual(['[{}]']);
+          expect(log).toEqual([]);
         });
 
 
         it('should watch array properties', function() {
           $rootScope.obj = [];
           $rootScope.$digest();
-          expect(log).toEqual(['[]']);
+          expect(log.empty()).toEqual([{newVal: [], oldVal: [], identical: true}]);
 
           $rootScope.obj.push('a');
           $rootScope.$digest();
-          expect(log).toEqual(['[]', '["a"]']);
+          expect(log.empty()).toEqual([{newVal: ['a'], oldVal: []}]);
 
           $rootScope.obj[0] = 'b';
           $rootScope.$digest();
-          expect(log).toEqual(['[]', '["a"]', '["b"]']);
+          expect(log.empty()).toEqual([{newVal: ['b'], oldVal: ['a']}]);
 
           $rootScope.obj.push([]);
           $rootScope.obj.push({});
-          log = [];
           $rootScope.$digest();
-          expect(log).toEqual(['["b",[],{}]']);
+          expect(log.empty()).toEqual([{newVal: ['b', [], {}], oldVal: ['b']}]);
 
           var temp = $rootScope.obj[1];
           $rootScope.obj[1] = $rootScope.obj[2];
           $rootScope.obj[2] = temp;
           $rootScope.$digest();
-          expect(log).toEqual([ '["b",[],{}]', '["b",{},[]]' ]);
+          expect(log.empty()).toEqual([{newVal: ['b', {}, []], oldVal: ['b', [], {}]}]);
 
           $rootScope.obj.shift();
-          log = [];
           $rootScope.$digest();
-          expect(log).toEqual([ '[{},[]]' ]);
+          expect(log.empty()).toEqual([{newVal: [{}, []], oldVal: ['b', {}, []]}]);
         });
 
+
         it('should watch array-like objects like arrays', function () {
           var arrayLikelog = [];
           $rootScope.$watchCollection('arrayLikeObject', function logger(obj) {
@@ -601,57 +624,72 @@ describe('Scope', function() {
 
 
       describe('object', function() {
+
+        it('should return oldCollection === newCollection only on the first listener call',
+            inject(function($rootScope, log) {
+
+          $rootScope.obj = {'a': 'b'};
+          // first time should be identical
+          $rootScope.$digest();
+          expect(log.empty()).toEqual([{newVal: {'a': 'b'}, oldVal: {'a': 'b'}, identical: true}]);
+
+          // second time not identical
+          $rootScope.obj.a = 'c';
+          $rootScope.$digest();
+          expect(log).toEqual([{newVal: {'a': 'c'}, oldVal: {'a': 'b'}}]);
+        }));
+
+
         it('should trigger when property changes into object', function() {
           $rootScope.obj = 'test';
           $rootScope.$digest();
-          expect(log).toEqual(['"test"']);
+          expect(log.empty()).toEqual([{newVal: 'test', oldVal: 'test', identical: true}]);
 
           $rootScope.obj = {};
           $rootScope.$digest();
-          expect(log).toEqual(['"test"', '{}']);
+          expect(log.empty()).toEqual([{newVal: {}, oldVal: 'test'}]);
         });
 
 
         it('should not trigger change when object in collection changes', function() {
           $rootScope.obj = {name: {}};
           $rootScope.$digest();
-          expect(log).toEqual(['{"name":{}}']);
+          expect(log.empty()).toEqual([{newVal: {name: {}}, oldVal: {name: {}}, identical: true}]);
 
           $rootScope.obj.name.bar = 'foo';
           $rootScope.$digest();
-          expect(log).toEqual(['{"name":{}}']);
+          expect(log.empty()).toEqual([]);
         });
 
 
         it('should watch object properties', function() {
           $rootScope.obj = {};
           $rootScope.$digest();
-          expect(log).toEqual(['{}']);
+          expect(log.empty()).toEqual([{newVal: {}, oldVal: {}, identical: true}]);
 
           $rootScope.obj.a= 'A';
           $rootScope.$digest();
-          expect(log).toEqual(['{}', '{"a":"A"}']);
+          expect(log.empty()).toEqual([{newVal: {a: 'A'}, oldVal: {}}]);
 
           $rootScope.obj.a = 'B';
           $rootScope.$digest();
-          expect(log).toEqual(['{}', '{"a":"A"}', '{"a":"B"}']);
+          expect(log.empty()).toEqual([{newVal: {a: 'B'}, oldVal: {a: 'A'}}]);
 
           $rootScope.obj.b = [];
           $rootScope.obj.c = {};
-          log = [];
           $rootScope.$digest();
-          expect(log).toEqual(['{"a":"B","b":[],"c":{}}']);
+          expect(log.empty()).toEqual([{newVal: {a: 'B', b: [], c: {}}, oldVal: {a: 'B'}}]);
 
           var temp = $rootScope.obj.a;
           $rootScope.obj.a = $rootScope.obj.b;
           $rootScope.obj.c = temp;
           $rootScope.$digest();
-          expect(log).toEqual([ '{"a":"B","b":[],"c":{}}', '{"a":[],"b":[],"c":"B"}' ]);
+          expect(log.empty()).
+              toEqual([{newVal: {a: [], b: {}, c: 'B'}, oldVal: {a: 'B', b: [], c: {}}}]);
 
           delete $rootScope.obj.a;
-          log = [];
           $rootScope.$digest();
-          expect(log).toEqual([ '{"b":[],"c":"B"}' ]);
+          expect(log.empty()).toEqual([{newVal: {b: {}, c: 'B'}, oldVal: {a: [], b: {}, c: 'B'}}]);
         });
       });
     });