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

Scope#$watchCollection oldValue == newValue fix #6736

Closed
wants to merge 2 commits into from
Closed

Scope#$watchCollection oldValue == newValue fix #6736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5e41a0
chore(log): add `log.empty()` method to the testing logger
IgorMinar Mar 17, 2014
78057a9
fix(Scope): $watchCollection should call listener with oldValue
IgorMinar Mar 17, 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
51 changes: 43 additions & 8 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use shallowCopy()? do we need to hang onto $$hashKey for the veryOldValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at shallowCopy, but since it doesn't support arrays I didn't use it.

I haven't thought much about $-prefixed properties. I'm not sure why they should be excluded in this case. Do you have some opinions on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, I was just thinking it would be (marginally) less code. But it's true that it doesn't support arrays (but arrays are using a different codepath anyways), and not caring about the '$' should improve perf by some negligible amount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's worth it. besides the $$hashkey might actually be useful for people trying to figure out what changed.

veryOldValue[key] = newValue[key];
}
}
}
}
}

return this.$watch($watchCollectionWatch, $watchCollectionAction);
Expand Down
14 changes: 10 additions & 4 deletions test/helpers/testabilityPatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,21 +269,27 @@ function provideLog($provide) {

log.toString = function() {
return messages.join('; ');
}
};

log.toArray = function() {
return messages;
}
};

log.reset = function() {
messages = [];
};

log.empty = function() {
var currentMessages = messages;
messages = [];
return currentMessages;
}

log.fn = function(msg) {
return function() {
log(msg);
}
}
};
};

log.$$log = true;

Expand Down
114 changes: 76 additions & 38 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
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