Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

watchCollection: Return correct oldCollection argument to listener #5661

Closed
wants to merge 6 commits into from
Closed

watchCollection: Return correct oldCollection argument to listener #5661

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
950c6f3
watchCollection: Return correct oldCollection argument to listener
johnsoftek Jan 7, 2014
ca6776a
watchCollection: Ensure that newCollection = oldCollection on first c…
Jan 7, 2014
42dd6a1
Refactor oldValue initialisation
Jan 7, 2014
69ea4bc
watchCollection: Re-order tests for clarity
Jan 7, 2014
34333fb
Ensure oldCollection === newCollection on first call (only) to listen…
Jan 8, 2014
b809444
Simplify oldCollection parameter checking
johnsoftek Mar 18, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 65 additions & 46 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,79 +416,98 @@ function $RootScopeProvider(){
var self = this;
var oldValue;
var newValue;
var changeDetected = 0;
var changeFlipFlop = 0;
var objGetter = $parse(obj);
var internalArray = [];
var internalObject = {};
var oldLength = 0;
var internalLength = 0;

// Holds simple value or reference to internalArray or internalObject.
// The special initial value is used to ensure that the listener is called
// when the watch is established and that oldValue = newValue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= -> ===

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Igor,
I have simplified the old value checking. Let me know what you think.

On 16 March 2014 02:21, Igor Minar notifications@github.com wrote:

In src/ng/rootScope.js:

     var objGetter = $parse(obj);
     var internalArray = [];
     var internalObject = {};

  •    var oldLength = 0;

  •    var internalLength = 0;

  •    // Holds simple value or reference to internalArray or internalObject.

  •    // The special initial value is used to ensure that the listener is called

  •    // when the watch is established and that oldValue = newValue.

= -> ===

Reply to this email directly or view it on GitHubhttps://github.com//pull/5661/files#r10635231
.

var internalValue = initWatchVal;

function $watchCollectionWatch() {
var newLength, key, i, changeDetected;

newValue = objGetter(self);
var newLength, key;
oldValue = internalValue;
changeDetected = 0;

if (!isObject(newValue)) {
if (oldValue !== newValue) {
oldValue = newValue;
if (internalValue !== newValue) {
internalValue = newValue;
changeDetected++;
}
} else if (isArrayLike(newValue)) {
if (oldValue !== internalArray) {
// we are transitioning from something which was not an array into array.
oldValue = internalArray;
oldLength = oldValue.length = 0;
changeDetected++;
}

newLength = newValue.length;

if (oldLength !== newLength) {
// if lengths do not match we need to trigger change notification
if (internalValue !== internalArray) {
// we are transitioning from something which was not an array into array.
changeDetected++;
oldValue.length = oldLength = newLength;
}
// copy the items to oldValue and look for changes.
for (var i = 0; i < newLength; i++) {
if (oldValue[i] !== newValue[i]) {
} else {
if (internalLength !== newLength) {
// if lengths do not match we need to trigger change notification
changeDetected++;
oldValue[i] = newValue[i];
} else {
// look for item changes
for (i = 0; i < newLength; i++) {
if (internalValue[i] !== newValue[i]) {
changeDetected++;
break;
}
}
}
}
if (changeDetected) {
// copy the items to array cache
internalValue = internalArray = [];
internalValue.length = internalLength = newLength;
for (i = 0; i < newLength; i++) {
internalValue[i] = newValue[i];
}
}
} else {
if (oldValue !== internalObject) {
// we are transitioning from something which was not an object into object.
oldValue = internalObject = {};
oldLength = 0;
if (internalValue !== internalObject) {
// we are transitioning from something which was not an object into object
changeDetected++;
}
// copy the items to oldValue and look for changes.
newLength = 0;
for (key in newValue) {
if (newValue.hasOwnProperty(key)) {
newLength++;
if (oldValue.hasOwnProperty(key)) {
if (oldValue[key] !== newValue[key]) {
} else {
// look for item changes
newLength = 0;
for (key in newValue) {
if (newValue.hasOwnProperty(key)) {
newLength++;
if (! (internalValue.hasOwnProperty(key) &&
internalValue[key] === newValue[key])) {
changeDetected++;
oldValue[key] = newValue[key];
break;
}
} else {
oldLength++;
oldValue[key] = newValue[key];
changeDetected++;
}
}
if (internalLength !== newLength) {
changeDetected++;
}
}
if (oldLength > newLength) {
// we used to have more keys, need to find them and destroy them.
changeDetected++;
for(key in oldValue) {
if (oldValue.hasOwnProperty(key) && !newValue.hasOwnProperty(key)) {
oldLength--;
delete oldValue[key];
if (changeDetected) {
// copy the items to object cache
internalValue = internalObject = {};
internalLength = 0;
for (key in newValue) {
if (newValue.hasOwnProperty(key)) {
internalLength++;
internalValue[key] = newValue[key];
}
}
}
}
return changeDetected;

if (changeDetected) {
changeFlipFlop = 1 - changeFlipFlop;
if (oldValue === initWatchVal) {
oldValue = newValue;
}
}

return changeFlipFlop;
}

function $watchCollectionAction() {
Expand Down
48 changes: 45 additions & 3 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,12 +456,36 @@ describe('Scope', function() {


describe('$watchCollection', function() {
var log, $rootScope, deregister;
var log, logb, $rootScope, deregister;

it('should return oldCollection === newCollection on first call (only) to listener', inject(function($rootScope) {
var paramsEqual;
$rootScope.obj = {'a': 'b'};
deregister = $rootScope.$watchCollection('obj', function listener(obj, objb) {
paramsEqual = (obj === objb);
});

// equal first time
$rootScope.$digest();
expect(paramsEqual).toBeTruthy();

$rootScope.obj.a = 'c';
$rootScope.$digest();
expect(paramsEqual).toBeFalsy();

$rootScope.obj = undefined;
$rootScope.$digest();
expect(paramsEqual).toBeFalsy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't use toBeFalsy or toBeTruthy in angular test suite. We prefer strict checks like toBe(true) or toBeUndefined() instead


}));


beforeEach(inject(function(_$rootScope_) {
log = [];
logb = []; // list of before values
log = []; // list of after values
$rootScope = _$rootScope_;
deregister = $rootScope.$watchCollection('obj', function logger(obj) {
deregister = $rootScope.$watchCollection('obj', function logger(obj, objb) {
logb.push(toJson(objb));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why reverse the order? (logb before log)? it's a bit surprising..

log.push(toJson(obj));
});
}));
Expand Down Expand Up @@ -494,22 +518,27 @@ describe('Scope', function() {
it('should trigger when property changes into array', function() {
$rootScope.obj = 'test';
$rootScope.$digest();
expect(logb).toEqual(['"test"']);
expect(log).toEqual(['"test"']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all these changes made the tests harder to understand. maybe having just a single logger that would log newValue and oldValue would be better. OR not change this logs at all and just write an explicit test that will verify that the old value is being correctly passed as an argument.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in any case we are missing this explicit test here. it's a must. just like you had the test for initial state, we need one for non initial state.


$rootScope.obj = [];
$rootScope.$digest();
expect(logb).toEqual(['"test"', '"test"']);
expect(log).toEqual(['"test"', '[]']);

$rootScope.obj = {};
$rootScope.$digest();
expect(logb).toEqual(['"test"', '"test"', '[]']);
expect(log).toEqual(['"test"', '[]', '{}']);

$rootScope.obj = [];
$rootScope.$digest();
expect(logb).toEqual(['"test"', '"test"', '[]', '{}']);
expect(log).toEqual(['"test"', '[]', '{}', '[]']);

$rootScope.obj = undefined;
$rootScope.$digest();
expect(logb).toEqual(['"test"', '"test"', '[]', '{}', '[]']);
expect(log).toEqual(['"test"', '[]', '{}', '[]', undefined]);
});

Expand All @@ -532,22 +561,27 @@ describe('Scope', function() {

$rootScope.obj.push('a');
$rootScope.$digest();
expect(logb).toEqual(['[]', '[]']);
expect(log).toEqual(['[]', '["a"]']);

$rootScope.obj[0] = 'b';
$rootScope.$digest();
expect(logb).toEqual(['[]', '[]', '["a"]']);
expect(log).toEqual(['[]', '["a"]', '["b"]']);

$rootScope.obj.push([]);
$rootScope.obj.push({});
logb = [];
log = [];
$rootScope.$digest();
expect(logb).toEqual(['["b"]']);
expect(log).toEqual(['["b",[],{}]']);

var temp = $rootScope.obj[1];
$rootScope.obj[1] = $rootScope.obj[2];
$rootScope.obj[2] = temp;
$rootScope.$digest();
expect(logb).toEqual([ '["b"]', '["b",[],{}]']);
expect(log).toEqual([ '["b",[],{}]', '["b",{},[]]' ]);

$rootScope.obj.shift();
Expand Down Expand Up @@ -583,6 +617,7 @@ describe('Scope', function() {

$rootScope.obj = {};
$rootScope.$digest();
expect(logb).toEqual(['"test"', '"test"']);
expect(log).toEqual(['"test"', '{}']);
});

Expand All @@ -605,27 +640,34 @@ describe('Scope', function() {

$rootScope.obj.a= 'A';
$rootScope.$digest();
expect(logb).toEqual(['{}', '{}']);
expect(log).toEqual(['{}', '{"a":"A"}']);

$rootScope.obj.a = 'B';
$rootScope.$digest();
expect(logb).toEqual(['{}', '{}', '{"a":"A"}']);
expect(log).toEqual(['{}', '{"a":"A"}', '{"a":"B"}']);

$rootScope.obj.b = [];
$rootScope.obj.c = {};
log = [];
logb = [];
$rootScope.$digest();
expect(logb).toEqual(['{"a":"B"}']);
expect(log).toEqual(['{"a":"B","b":[],"c":{}}']);

var temp = $rootScope.obj.a;
$rootScope.obj.a = $rootScope.obj.b;
$rootScope.obj.c = temp;
$rootScope.$digest();
expect(logb).toEqual([ '{"a":"B"}', '{"a":"B","b":[],"c":{}}']);
expect(log).toEqual([ '{"a":"B","b":[],"c":{}}', '{"a":[],"b":[],"c":"B"}' ]);

delete $rootScope.obj.a;
logb = [];
log = [];
$rootScope.$digest();
expect(logb).toEqual([ '{"a":[],"b":[],"c":"B"}' ]);
expect(log).toEqual([ '{"b":[],"c":"B"}' ]);
});
});
Expand Down