Skip to content

Commit cbc3acb

Browse files
IgorMinarIgorMinar
committed
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 angular#2621 Closes angular#5661 Closes angular#5688
1 parent 84a9b45 commit cbc3acb

File tree

2 files changed

+112
-41
lines changed

2 files changed

+112
-41
lines changed

‎src/ng/rootScope.js

+36-3
Original file line numberDiff line numberDiff line change
@@ -409,19 +409,27 @@ function $RootScopeProvider(){
409409
*/
410410
$watchCollection: function(obj, listener) {
411411
var self = this;
412-
var oldValue;
412+
// the current value, updated on each dirty-check run
413413
var newValue;
414+
// a shallow copy of the newValue from the last dirty-check run,
415+
// updated to match newValue during dirty-check run
416+
var oldValue;
417+
// a shallow copy of the newValue from when the last change happened
418+
var veryOldValue;
419+
// only track veryOldValue if the listener is asking for it
420+
var trackVeryOldValue = (listener.length > 1);
414421
var changeDetected = 0;
415422
var objGetter = $parse(obj);
416423
var internalArray = [];
417424
var internalObject = {};
425+
var initRun = true;
418426
var oldLength = 0;
419427

420428
function $watchCollectionWatch() {
421429
newValue = objGetter(self);
422430
var newLength, key;
423431

424-
if (!isObject(newValue)) {
432+
if (!isObject(newValue)) { // if primitive
425433
if (oldValue !== newValue) {
426434
oldValue = newValue;
427435
changeDetected++;
@@ -487,7 +495,32 @@ function $RootScopeProvider(){
487495
}
488496

489497
function $watchCollectionAction() {
490-
listener(newValue, oldValue, self);
498+
if (initRun) {
499+
initRun = false;
500+
listener(newValue, newValue, self);
501+
} else {
502+
listener(newValue, veryOldValue, self);
503+
}
504+
505+
// make a copy for the next time a collection is changed
506+
if (trackVeryOldValue) {
507+
if (!isObject(newValue)) {
508+
//primitive
509+
veryOldValue = newValue;
510+
} else if (isArrayLike(newValue)) {
511+
veryOldValue = new Array(newValue.length);
512+
for (var i = 0; i < newValue.length; i++) {
513+
veryOldValue[i] = newValue[i];
514+
}
515+
} else { // if object
516+
veryOldValue = {};
517+
for (var key in newValue) {
518+
if (hasOwnProperty.call(newValue, key)) {
519+
veryOldValue[key] = newValue[key];
520+
}
521+
}
522+
}
523+
}
491524
}
492525

493526
return this.$watch($watchCollectionWatch, $watchCollectionAction);

‎test/ng/rootScopeSpec.js

+76-38
Original file line numberDiff line numberDiff line change
@@ -483,104 +483,127 @@ describe('Scope', function() {
483483
describe('$watchCollection', function() {
484484
var log, $rootScope, deregister;
485485

486-
beforeEach(inject(function(_$rootScope_) {
487-
log = [];
486+
beforeEach(inject(function(_$rootScope_, _log_) {
488487
$rootScope = _$rootScope_;
489-
deregister = $rootScope.$watchCollection('obj', function logger(obj) {
490-
log.push(toJson(obj));
488+
log = _log_;
489+
deregister = $rootScope.$watchCollection('obj', function logger(newVal, oldVal) {
490+
var msg = {newVal: newVal, oldVal: oldVal};
491+
492+
if (newVal === oldVal) {
493+
msg.identical = true;
494+
}
495+
496+
log(msg);
491497
});
492498
}));
493499

494500

495501
it('should not trigger if nothing change', inject(function($rootScope) {
496502
$rootScope.$digest();
497-
expect(log).toEqual([undefined]);
503+
expect(log).toEqual([{ newVal : undefined, oldVal : undefined, identical : true }]);
504+
log.reset();
498505

499506
$rootScope.$digest();
500-
expect(log).toEqual([undefined]);
507+
expect(log).toEqual([]);
501508
}));
502509

503510

504-
it('should allow deregistration', inject(function($rootScope) {
511+
it('should allow deregistration', function() {
505512
$rootScope.obj = [];
506513
$rootScope.$digest();
507-
508-
expect(log).toEqual(['[]']);
514+
expect(log.toArray().length).toBe(1);
515+
log.reset();
509516

510517
$rootScope.obj.push('a');
511518
deregister();
512519

513520
$rootScope.$digest();
514-
expect(log).toEqual(['[]']);
515-
}));
521+
expect(log).toEqual([]);
522+
});
516523

517524

518525
describe('array', function() {
526+
527+
it('should return oldCollection === newCollection only on the first listener call',
528+
inject(function($rootScope, log) {
529+
530+
// first time should be identical
531+
$rootScope.obj = ['a', 'b'];
532+
$rootScope.$digest();
533+
expect(log).toEqual([{newVal: ['a', 'b'], oldVal: ['a', 'b'], identical: true}]);
534+
log.reset();
535+
536+
// second time should be different
537+
$rootScope.obj[1] = 'c';
538+
$rootScope.$digest();
539+
expect(log).toEqual([{newVal: ['a', 'c'], oldVal: ['a', 'b']}]);
540+
}));
541+
542+
519543
it('should trigger when property changes into array', function() {
520544
$rootScope.obj = 'test';
521545
$rootScope.$digest();
522-
expect(log).toEqual(['"test"']);
546+
expect(log.empty()).toEqual([{newVal: "test", oldVal: "test", identical: true}]);
523547

524548
$rootScope.obj = [];
525549
$rootScope.$digest();
526-
expect(log).toEqual(['"test"', '[]']);
550+
expect(log.empty()).toEqual([{newVal: [], oldVal: "test"}]);
527551

528552
$rootScope.obj = {};
529553
$rootScope.$digest();
530-
expect(log).toEqual(['"test"', '[]', '{}']);
554+
expect(log.empty()).toEqual([{newVal: {}, oldVal: []}]);
531555

532556
$rootScope.obj = [];
533557
$rootScope.$digest();
534-
expect(log).toEqual(['"test"', '[]', '{}', '[]']);
558+
expect(log.empty()).toEqual([{newVal: [], oldVal: {}}]);
535559

536560
$rootScope.obj = undefined;
537561
$rootScope.$digest();
538-
expect(log).toEqual(['"test"', '[]', '{}', '[]', undefined]);
562+
expect(log.empty()).toEqual([{newVal: undefined, oldVal: []}]);
539563
});
540564

541565

542566
it('should not trigger change when object in collection changes', function() {
543567
$rootScope.obj = [{}];
544568
$rootScope.$digest();
545-
expect(log).toEqual(['[{}]']);
569+
expect(log.empty()).toEqual([{newVal: [{}], oldVal: [{}], identical: true}]);
546570

547571
$rootScope.obj[0].name = 'foo';
548572
$rootScope.$digest();
549-
expect(log).toEqual(['[{}]']);
573+
expect(log).toEqual([]);
550574
});
551575

552576

553577
it('should watch array properties', function() {
554578
$rootScope.obj = [];
555579
$rootScope.$digest();
556-
expect(log).toEqual(['[]']);
580+
expect(log.empty()).toEqual([{newVal: [], oldVal: [], identical: true}]);
557581

558582
$rootScope.obj.push('a');
559583
$rootScope.$digest();
560-
expect(log).toEqual(['[]', '["a"]']);
584+
expect(log.empty()).toEqual([{newVal: ['a'], oldVal: []}]);
561585

562586
$rootScope.obj[0] = 'b';
563587
$rootScope.$digest();
564-
expect(log).toEqual(['[]', '["a"]', '["b"]']);
588+
expect(log.empty()).toEqual([{newVal: ['b'], oldVal: ['a']}]);
565589

566590
$rootScope.obj.push([]);
567591
$rootScope.obj.push({});
568-
log = [];
569592
$rootScope.$digest();
570-
expect(log).toEqual(['["b",[],{}]']);
593+
expect(log.empty()).toEqual([{newVal: ['b', [], {}], oldVal: ['b']}]);
571594

572595
var temp = $rootScope.obj[1];
573596
$rootScope.obj[1] = $rootScope.obj[2];
574597
$rootScope.obj[2] = temp;
575598
$rootScope.$digest();
576-
expect(log).toEqual([ '["b",[],{}]', '["b",{},[]]' ]);
599+
expect(log.empty()).toEqual([{newVal: ['b', {}, []], oldVal: ['b', [], {}]}]);
577600

578601
$rootScope.obj.shift();
579-
log = [];
580602
$rootScope.$digest();
581-
expect(log).toEqual([ '[{},[]]' ]);
603+
expect(log.empty()).toEqual([{newVal: [{}, []], oldVal: ['b', {}, []]}]);
582604
});
583605

606+
584607
it('should watch array-like objects like arrays', function () {
585608
var arrayLikelog = [];
586609
$rootScope.$watchCollection('arrayLikeObject', function logger(obj) {
@@ -601,57 +624,72 @@ describe('Scope', function() {
601624

602625

603626
describe('object', function() {
627+
628+
it('should return oldCollection === newCollection only on the first listener call',
629+
inject(function($rootScope, log) {
630+
631+
$rootScope.obj = {'a': 'b'};
632+
// first time should be identical
633+
$rootScope.$digest();
634+
expect(log.empty()).toEqual([{newVal: {'a': 'b'}, oldVal: {'a': 'b'}, identical: true}]);
635+
636+
// second time not identical
637+
$rootScope.obj.a = 'c';
638+
$rootScope.$digest();
639+
expect(log).toEqual([{newVal: {'a': 'c'}, oldVal: {'a': 'b'}}]);
640+
}));
641+
642+
604643
it('should trigger when property changes into object', function() {
605644
$rootScope.obj = 'test';
606645
$rootScope.$digest();
607-
expect(log).toEqual(['"test"']);
646+
expect(log.empty()).toEqual([{newVal: 'test', oldVal: 'test', identical: true}]);
608647

609648
$rootScope.obj = {};
610649
$rootScope.$digest();
611-
expect(log).toEqual(['"test"', '{}']);
650+
expect(log.empty()).toEqual([{newVal: {}, oldVal: 'test'}]);
612651
});
613652

614653

615654
it('should not trigger change when object in collection changes', function() {
616655
$rootScope.obj = {name: {}};
617656
$rootScope.$digest();
618-
expect(log).toEqual(['{"name":{}}']);
657+
expect(log.empty()).toEqual([{newVal: {name: {}}, oldVal: {name: {}}, identical: true}]);
619658

620659
$rootScope.obj.name.bar = 'foo';
621660
$rootScope.$digest();
622-
expect(log).toEqual(['{"name":{}}']);
661+
expect(log.empty()).toEqual([]);
623662
});
624663

625664

626665
it('should watch object properties', function() {
627666
$rootScope.obj = {};
628667
$rootScope.$digest();
629-
expect(log).toEqual(['{}']);
668+
expect(log.empty()).toEqual([{newVal: {}, oldVal: {}, identical: true}]);
630669

631670
$rootScope.obj.a= 'A';
632671
$rootScope.$digest();
633-
expect(log).toEqual(['{}', '{"a":"A"}']);
672+
expect(log.empty()).toEqual([{newVal: {a: 'A'}, oldVal: {}}]);
634673

635674
$rootScope.obj.a = 'B';
636675
$rootScope.$digest();
637-
expect(log).toEqual(['{}', '{"a":"A"}', '{"a":"B"}']);
676+
expect(log.empty()).toEqual([{newVal: {a: 'B'}, oldVal: {a: 'A'}}]);
638677

639678
$rootScope.obj.b = [];
640679
$rootScope.obj.c = {};
641-
log = [];
642680
$rootScope.$digest();
643-
expect(log).toEqual(['{"a":"B","b":[],"c":{}}']);
681+
expect(log.empty()).toEqual([{newVal: {a: 'B', b: [], c: {}}, oldVal: {a: 'B'}}]);
644682

645683
var temp = $rootScope.obj.a;
646684
$rootScope.obj.a = $rootScope.obj.b;
647685
$rootScope.obj.c = temp;
648686
$rootScope.$digest();
649-
expect(log).toEqual([ '{"a":"B","b":[],"c":{}}', '{"a":[],"b":[],"c":"B"}' ]);
687+
expect(log.empty()).
688+
toEqual([{newVal: {a: [], b: {}, c: 'B'}, oldVal: {a: 'B', b: [], c: {}}}]);
650689

651690
delete $rootScope.obj.a;
652-
log = [];
653691
$rootScope.$digest();
654-
expect(log).toEqual([ '{"b":[],"c":"B"}' ]);
692+
expect(log.empty()).toEqual([{newVal: {b: {}, c: 'B'}, oldVal: {a: [], b: {}, c: 'B'}}]);
655693
});
656694
});
657695
});

0 commit comments

Comments
 (0)