From 78057a945ef84cbb05f9417fe884cb8c28e67b44 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Mon, 17 Mar 2014 15:48:09 -0700 Subject: [PATCH] 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 --- src/ng/rootScope.js | 51 +++++++++++++++--- test/ng/rootScopeSpec.js | 114 ++++++++++++++++++++++++++------------- 2 files changed, 119 insertions(+), 46 deletions(-) diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 2bb965bb7b6f..ce0f8ad54cb3 100644 --- a/src/ng/rootScope.js +++ b/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); diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index f9cf9412c605..251a8ce882c4 100644 --- a/test/ng/rootScopeSpec.js +++ b/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'}}]); }); }); });