Skip to content

Commit

Permalink
fix(rootScope): $watchCollection returns in oldCollection a copy of t…
Browse files Browse the repository at this point in the history
…he former array data

related to angular#1751
When watching arrays, $watchCollection returned the new data both in the newCollection
and the oldCollection arguments of the listener
  • Loading branch information
Gonzalo Ruiz de Villa committed Sep 4, 2013
1 parent 40c0220 commit bf69ca0
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/ng/rootScope.js
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ function $RootScopeProvider(){
$watchCollection: function(obj, listener) {
var self = this;
var oldValue;
var oldArray;
var newValue;
var changeDetected = 0;
var objGetter = $parse(obj);
Expand All @@ -371,6 +372,7 @@ function $RootScopeProvider(){

function $watchCollectionWatch() {
newValue = objGetter(self);
oldArray = null;
var newLength, key;

if (!isObject(newValue)) {
Expand All @@ -386,6 +388,8 @@ function $RootScopeProvider(){
changeDetected++;
}

oldArray = oldValue.slice(0);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 7, 2014

this is going to be a perf penalty: http://jsperf.com/slice-0

could you check if listener.length > 1 and slice the array only then?

btw what about objects/maps? slice is not going to work on those.

This comment has been minimized.

Copy link
@gonzaloruizdevilla

gonzaloruizdevilla Jan 7, 2014

Owner

Array.prototype.slice.call(oldValue, 0) will work on objects/maps. I have added a test for this case.
I have rebased and updated this commit in 767ee85

Speaking about the perf penalty on big arrays, I don't think is going to be that big. I mean, it is there, but taking into account that slice is one of the steps, the penalty is not going to be so huge as your jsperf may imply: http://jsperf.com/slice-performance-penalty
Iterating over the arrays for element comparison takes time of same order of magnitude as the slice.


newLength = newValue.length;

if (oldLength !== newLength) {
Expand Down Expand Up @@ -439,7 +443,7 @@ function $RootScopeProvider(){
}

function $watchCollectionAction() {
listener(newValue, oldValue, self);
listener(newValue, oldArray || oldValue, self);
}

return this.$watch($watchCollectionWatch, $watchCollectionAction);
Expand Down
22 changes: 22 additions & 0 deletions test/ng/rootScopeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,28 @@ describe('Scope', function() {
$rootScope.$digest();
expect(arrayLikelog).toEqual(['x', 'y']);
});

it('should return a new array with old values', function(){
var watchArgs;
$rootScope.$watchCollection('obj', function (newValues, oldValues) {
watchArgs = {
newValues: newValues,
oldValues: oldValues
};
});

$rootScope.obj = ['a'];
$rootScope.$digest();

expect(watchArgs.newValues).toEqual($rootScope.obj);
expect(watchArgs.oldValues).toEqual([]);

$rootScope.obj.push('b');
$rootScope.$digest();

expect(watchArgs.newValues).toEqual(['a', 'b']);
expect(watchArgs.oldValues).toEqual(['a']);
})
});


Expand Down

0 comments on commit bf69ca0

Please sign in to comment.