Skip to content

Commit

Permalink
fix(Scope): $watchCollection should call listener with oldValue
Browse files Browse the repository at this point in the history 
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 angular#2621
Closes angular#5661
Closes angular#5688
  • Loading branch information
@IgorMinar
IgorMinar committed Mar 18, 2014
1 parent 84a9b45 commit cbc3acb
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 41 deletions.
39 changes: 36 additions & 3 deletions src/ng/rootScope.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,19 +409,27 @@ function $RootScopeProvider(){
*/
$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);

This comment has been minimized.

Sign in to view

Copy link
@Narretz

Narretz Mar 18, 2014

As you said before, this breaks a watch listener that uses arguments Is this worth mentioning in the docs / inline comment? The likelihood that somebody trips over this is pretty small (considering that the intial bug is very old, and very few people reported it), but if someone hits it ...
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++;
Expand Down Expand Up @@ -487,7 +495,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);
Expand Down
114 changes: 76 additions & 38 deletions test/ng/rootScopeSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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'}}]);
});
});
});
Expand Down

0 comments on commit cbc3acb

Please sign in to comment.