From e07dd382169da0e29d6be05e777dec10b05314d5 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Mon, 24 Oct 2011 09:18:44 -0700 Subject: [PATCH 1/2] fix(ng:repeat) with array ignore properties not representing array elements Along the way I also changed the repeater impl to use for loop instead of for in loop. Iteration over objects is handled by creating an array of keys, which is sorted and this array then determines the order of iteration over an element. This makes repeating over objects deterministic and cross-browser compatible. --- src/widgets.js | 98 +++++++++++++++++++++++++-------------------- test/widgetsSpec.js | 24 ++++++++++- 2 files changed, 77 insertions(+), 45 deletions(-) diff --git a/src/widgets.js b/src/widgets.js index 423fe6dd69c9..f8da15b3ad40 100644 --- a/src/widgets.js +++ b/src/widgets.js @@ -361,7 +361,7 @@ angularWidget('@ng:repeat', function(expression, element){ // We expect this to be a rare case. var lastOrder = new HashQueueMap(); this.$watch(function(scope){ - var index = 0, + var index, length, collection = scope.$eval(rhs), collectionLength = size(collection, true), childScope, @@ -372,52 +372,64 @@ angularWidget('@ng:repeat', function(expression, element){ array, last, // last object information {scope, element, index} cursor = iterStartElement; // current position of the node - for (key in collection) { - if (collection.hasOwnProperty(key) && key.charAt(0) != '$') { - last = lastOrder.shift(value = collection[key]); - if (last) { - // if we have already seen this object, then we need to reuse the - // associated scope/element - childScope = last.scope; - nextOrder.push(value, last); - - if (index === last.index) { - // do nothing - cursor = last.element; - } else { - // existing item which got moved - last.index = index; - // This may be a noop, if the element is next, but I don't know of a good way to - // figure this out, since it would require extra DOM access, so let's just hope that - // the browsers realizes that it is noop, and treats it as such. - cursor.after(last.element); - cursor = last.element; - } - } else { - // new item which we don't know about - childScope = parentScope.$new(); + if (!isArray(collection)) { + // if object, extract keys, sort them and use to determine order of iteration over obj props + array = []; + forEach(collection, function(value, key) { + if (collection.hasOwnProperty(key) && key.charAt(0) != '$') { + array.push(key); } + }); + array.sort(); + } else { + array = collection || []; + } - childScope[valueIdent] = collection[key]; - if (keyIdent) childScope[keyIdent] = key; - childScope.$index = index; - childScope.$position = index == 0 - ? 'first' - : (index == collectionLength - 1 ? 'last' : 'middle'); - - if (!last) { - linker(childScope, function(clone){ - cursor.after(clone); - last = { - scope: childScope, - element: (cursor = clone), - index: index - }; - nextOrder.push(value, last); - }); + // we are not using forEach for perf reasons (trying to avoid #call) + for (index = 0, length = array.length; index < length; index++) { + key = (collection === array) ? index : array[index]; + value = collection[key]; + last = lastOrder.shift(value); + if (last) { + // if we have already seen this object, then we need to reuse the + // associated scope/element + childScope = last.scope; + nextOrder.push(value, last); + + if (index === last.index) { + // do nothing + cursor = last.element; + } else { + // existing item which got moved + last.index = index; + // This may be a noop, if the element is next, but I don't know of a good way to + // figure this out, since it would require extra DOM access, so let's just hope that + // the browsers realizes that it is noop, and treats it as such. + cursor.after(last.element); + cursor = last.element; } + } else { + // new item which we don't know about + childScope = parentScope.$new(); + } - index ++; + childScope[valueIdent] = value; + if (keyIdent) childScope[keyIdent] = key; + childScope.$index = index; + childScope.$position = index == 0 + ? 'first' + : (index == collectionLength - 1 ? 'last' : 'middle'); + + if (!last) { + linker(childScope, function(clone){ + cursor.after(clone); + last = { + scope: childScope, + element: (cursor = clone), + index: index + }; + nextOrder.push(value, last); + }); } } diff --git a/test/widgetsSpec.js b/test/widgetsSpec.js index 905553258898..c1ecbbb3ee91 100644 --- a/test/widgetsSpec.js +++ b/test/widgetsSpec.js @@ -254,7 +254,7 @@ describe("widget", function() { 'ng:bind="key + \':\' + val + $index + \'|\'">'); scope.items = {'misko':'m', 'shyam':'s', 'frodo':'f'}; scope.$digest(); - expect(element.text()).toEqual('misko:m0|shyam:s1|frodo:f2|'); + expect(element.text()).toEqual('frodo:f0|misko:m1|shyam:s2|'); }); it('should expose iterator position as $position when iterating over arrays', function() { @@ -282,7 +282,7 @@ describe("widget", function() { ''); scope.items = {'misko':'m', 'shyam':'s', 'doug':'d', 'frodo':'f'}; scope.$digest(); - expect(element.text()).toEqual('misko:m:first|shyam:s:middle|doug:d:middle|frodo:f:last|'); + expect(element.text()).toEqual('doug:d:first|frodo:f:middle|misko:m:middle|shyam:s:last|'); delete scope.items.doug; delete scope.items.frodo; @@ -312,6 +312,26 @@ describe("widget", function() { expect(element.text()).toEqual('a|b|Xc|d|X'); }); + it('should ignore non-array element properties when iterating over an array', function() { + var scope = compile(''); + scope.array = ['a', 'b', 'c']; + scope.array.foo = '23'; + scope.array.bar = function() {}; + scope.$digest(); + + expect(element.text()).toBe('a|b|c|'); + }); + + it('should iterate over non-existent elements of a sparse array', function() { + var scope = compile(''); + scope.array = ['a', 'b']; + scope.array[4] = 'c'; + scope.array[6] = 'd'; + scope.$digest(); + + expect(element.text()).toBe('a|b|||c||d|'); + }); + describe('stability', function() { var a, b, c, d, scope, lis; From ffb9d62ae881aba54b6b0bd884925418387d6f98 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Mon, 24 Oct 2011 22:49:30 -0700 Subject: [PATCH 2/2] style(HashQueueMap): fixing a typo in the comment --- src/apis.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apis.js b/src/apis.js index c9b2a99f4760..c23f9a614f51 100644 --- a/src/apis.js +++ b/src/apis.js @@ -936,7 +936,7 @@ HashMap.prototype = { }; /** - * A map where multiple values can be added to the same key such that the form a queue. + * A map where multiple values can be added to the same key such that they form a queue. * @returns {HashQueueMap} */ function HashQueueMap() {}