From ff53fea74e82bee3c24dd4ac3c5868be000405ad Mon Sep 17 00:00:00 2001 From: Zhenbo Zhang Date: Wed, 9 May 2012 00:00:38 +0300 Subject: [PATCH 1/4] fix:ng-repeat does not work with primitive types --- src/apis.js | 12 +++- src/ng/directive/ngRepeat.js | 27 +++++++- test/ApiSpecs.js | 4 ++ test/ng/directive/ngRepeatSpec.js | 106 ++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 4 deletions(-) diff --git a/src/apis.js b/src/apis.js index 7b4708020f3a..8965321ed3ba 100644 --- a/src/apis.js +++ b/src/apis.js @@ -97,5 +97,15 @@ HashQueueMap.prototype = { return array.shift(); } } - } + }, + + /** + * return the first item without deleting it + */ + peek: function(key) { + var array = this[key = hashKey(key)]; + if (array) { + return array[0]; + } +} }; diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 66464c1b8650..1538f3cbc564 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -89,6 +89,7 @@ var ngRepeatDirective = ngDirective({ // We need an array of these objects since the same object can be returned from the iterator. // We expect this to be a rare case. var lastOrder = new HashQueueMap(); + var indexValues = []; scope.$watch(function(scope){ var index, length, collection = scope.$eval(rhs), @@ -118,7 +119,20 @@ var ngRepeatDirective = ngDirective({ for (index = 0, length = array.length; index < length; index++) { key = (collection === array) ? index : array[index]; value = collection[key]; - last = lastOrder.shift(value); + + // if collection is array and value is object, it can be shifted to allow for position change + // if collection is array and value is not object, need to first check whether index is same to + // avoid shifting wrong value + // if collection is not array, need to always check index to avoid shifting wrong value + if (lastOrder.peek(value)) { + last = collection === array ? + ((typeof value == 'object') ? lastOrder.shift(value) : + (index === lastOrder.peek(value).index ? lastOrder.shift(value) : undefined)) : + (index === lastOrder.peek(value).index ? lastOrder.shift(value) : undefined); + } else { + last = undefined; + } + if (last) { // if we have already seen this object, then we need to reuse the // associated scope/element @@ -138,6 +152,12 @@ var ngRepeatDirective = ngDirective({ cursor = last.element; } } else { + if (indexValues[index] !== undefined && collection !== array) { + var preValue = indexValues[index]; + var v = lastOrder.shift(preValue); + v.element.remove(); + v.scope.$destroy(); + } // new item which we don't know about childScope = scope.$new(); } @@ -158,11 +178,12 @@ var ngRepeatDirective = ngDirective({ index: index }; nextOrder.push(value, last); + indexValues[index] = value; }); } } - - //shrink children + + //shrink children for (key in lastOrder) { if (lastOrder.hasOwnProperty(key)) { array = lastOrder[key]; diff --git a/test/ApiSpecs.js b/test/ApiSpecs.js index 35a85bd4e2a2..1e52cf442b7a 100644 --- a/test/ApiSpecs.js +++ b/test/ApiSpecs.js @@ -31,7 +31,11 @@ describe('api', function() { map.push('key', 'a'); map.push('key', 'b'); expect(map[hashKey('key')]).toEqual(['a', 'b']); + expect(map.peek('key')).toEqual('a'); + expect(map[hashKey('key')]).toEqual(['a', 'b']); expect(map.shift('key')).toEqual('a'); + expect(map.peek('key')).toEqual('b'); + expect(map[hashKey('key')]).toEqual(['b']); expect(map.shift('key')).toEqual('b'); expect(map.shift('key')).toEqual(undefined); expect(map[hashKey('key')]).toEqual(undefined); diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 5cb7196ba3c7..5b6ef2e79fc6 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -36,6 +36,85 @@ describe('ngRepeat', function() { expect(element.text()).toEqual('brad;'); })); + it('should ngRepeat over array of primitive correctly', inject(function($rootScope, $compile) { + element = $compile( + '')($rootScope); + + Array.prototype.extraProperty = "should be ignored"; + // INIT + $rootScope.items = [true, true, true]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('true;true;true;'); + delete Array.prototype.extraProperty; + + $rootScope.items = [false, true, true]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('false;true;true;'); + + $rootScope.items = [false, true, false]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('false;true;false;'); + + $rootScope.items = [true]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(1); + expect(element.text()).toEqual('true;'); + + $rootScope.items = [true, true, false]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('true;true;false;'); + + $rootScope.items = [true, false, false]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('true;false;false;'); + + // string + $rootScope.items = ['a', 'a', 'a']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('a;a;a;'); + + $rootScope.items = ['ab', 'a', 'a']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('ab;a;a;'); + + $rootScope.items = ['test']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(1); + expect(element.text()).toEqual('test;'); + + $rootScope.items = ['same', 'value']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(2); + expect(element.text()).toEqual('same;value;'); + + // number + $rootScope.items = ['12', '12', '12']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('12;12;12;'); + + $rootScope.items = ['53', '12', '27']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('53;12;27;'); + + $rootScope.items = ['89']; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(1); + expect(element.text()).toEqual('89;'); + + })); + + it('should ngRepeat over object', inject(function($rootScope, $compile) { element = $compile( @@ -46,6 +125,33 @@ describe('ngRepeat', function() { $rootScope.$digest(); expect(element.text()).toEqual('misko:swe;shyam:set;'); })); + + it('should ngRepeat over object with primitive value correctly', inject(function($rootScope, $compile) { + element = $compile( + '')($rootScope); + $rootScope.items = {misko:'true', shyam:'true', zhenbo: 'true'}; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('misko:true;shyam:true;zhenbo:true;'); + + $rootScope.items = {misko:'false', shyam:'true', zhenbo: 'true'}; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('misko:false;shyam:true;zhenbo:true;'); + + $rootScope.items = {misko:'false', shyam:'false', zhenbo: 'false'}; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(3); + expect(element.text()).toEqual('misko:false;shyam:false;zhenbo:false;'); + + $rootScope.items = {misko:'true'}; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(1); + expect(element.text()).toEqual('misko:true;'); + + })); it('should not ngRepeat over parent properties', inject(function($rootScope, $compile) { From e8d5f7e312c72a83118a59619973e76675c00855 Mon Sep 17 00:00:00 2001 From: Zhenbo Zhang Date: Wed, 9 May 2012 21:39:49 +0300 Subject: [PATCH 2/4] code update --- src/apis.js | 4 ++-- src/ng/directive/ngRepeat.js | 9 +++++++-- test/ng/directive/ngRepeatSpec.js | 22 +++++++++++++--------- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/apis.js b/src/apis.js index 8965321ed3ba..ee64493ab51d 100644 --- a/src/apis.js +++ b/src/apis.js @@ -99,7 +99,7 @@ HashQueueMap.prototype = { } }, - /** + /** * return the first item without deleting it */ peek: function(key) { @@ -107,5 +107,5 @@ HashQueueMap.prototype = { if (array) { return array[0]; } -} + } }; diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 1538f3cbc564..dafe97c1db42 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -126,7 +126,7 @@ var ngRepeatDirective = ngDirective({ // if collection is not array, need to always check index to avoid shifting wrong value if (lastOrder.peek(value)) { last = collection === array ? - ((typeof value == 'object') ? lastOrder.shift(value) : + ((isObject(value)) ? lastOrder.shift(value) : (index === lastOrder.peek(value).index ? lastOrder.shift(value) : undefined)) : (index === lastOrder.peek(value).index ? lastOrder.shift(value) : undefined); } else { @@ -152,7 +152,7 @@ var ngRepeatDirective = ngDirective({ cursor = last.element; } } else { - if (indexValues[index] !== undefined && collection !== array) { + if (indexValues.hasOwnProperty(index) && collection !== array) { var preValue = indexValues[index]; var v = lastOrder.shift(preValue); v.element.remove(); @@ -182,6 +182,11 @@ var ngRepeatDirective = ngDirective({ }); } } + + var i, l; + for (i = 0, l = indexValues.length - length; i < l; i++) { + indexValues.pop(); + } //shrink children for (key in lastOrder) { diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 5b6ef2e79fc6..b9f5ace810c6 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -36,11 +36,12 @@ describe('ngRepeat', function() { expect(element.text()).toEqual('brad;'); })); + it('should ngRepeat over array of primitive correctly', inject(function($rootScope, $compile) { element = $compile( - '')($rootScope); + '')($rootScope); Array.prototype.extraProperty = "should be ignored"; // INIT @@ -97,25 +98,23 @@ describe('ngRepeat', function() { expect(element.text()).toEqual('same;value;'); // number - $rootScope.items = ['12', '12', '12']; + $rootScope.items = [12, 12, 12]; $rootScope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('12;12;12;'); - $rootScope.items = ['53', '12', '27']; + $rootScope.items = [53, 12, 27]; $rootScope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('53;12;27;'); - $rootScope.items = ['89']; + $rootScope.items = [89]; $rootScope.$digest(); expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('89;'); - })); - it('should ngRepeat over object', inject(function($rootScope, $compile) { element = $compile( '')($rootScope); $rootScope.items = {misko:'true', shyam:'true', zhenbo: 'true'}; $rootScope.$digest(); - expect(element.find('li').length).toEqual(3); + expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('misko:true;shyam:true;zhenbo:true;'); - $rootScope.items = {misko:'false', shyam:'true', zhenbo: 'true'}; + $rootScope.items = {misko:'false', shyam:'true', zhenbo: 'true'}; $rootScope.$digest(); - expect(element.find('li').length).toEqual(3); + expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('misko:false;shyam:true;zhenbo:true;'); - $rootScope.items = {misko:'false', shyam:'false', zhenbo: 'false'}; + $rootScope.items = {misko:'false', shyam:'false', zhenbo: 'false'}; $rootScope.$digest(); - expect(element.find('li').length).toEqual(3); + expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('misko:false;shyam:false;zhenbo:false;'); - $rootScope.items = {misko:'true'}; + $rootScope.items = {misko:'true'}; $rootScope.$digest(); - expect(element.find('li').length).toEqual(1); + expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('misko:true;'); $rootScope.items = {shyam:'true', zhenbo: 'false'}; From a18a838d5c762a27469e15e443613dae72feb62f Mon Sep 17 00:00:00 2001 From: Zhenbo Zhang Date: Wed, 9 May 2012 22:06:50 +0300 Subject: [PATCH 4/4] one more test --- test/ng/directive/ngRepeatSpec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 1c9d20bcddd7..2e0b11b0d82a 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -112,6 +112,11 @@ describe('ngRepeat', function() { $rootScope.$digest(); expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('89;'); + + $rootScope.items = [89, 23]; + $rootScope.$digest(); + expect(element.find('li').length).toEqual(2); + expect(element.text()).toEqual('89;23;'); }));