ng-repeat : Fix issues when iterating over primitives

[petebacondarwin]

ng-repeat Directive

This complete rewrite of ng-repeat tracks element/scope pairing for objects since they can be tracked by object identity but tracks element/scope pairings for primitive values by the index position in the collection since there is no consistent way of tracking if a primitive has moved.

Fixes primitive issues

It solves various problems including:

• Input element losing focus on each change. See this broken Plnker based on master - http://plnkr.co/edit/bFxHW4?p=preview - and this fixed version - http://plnkr.co/edit/zh5P9Y?p=preview.
• Incorrect model updating when item is a primitive. Fixes ng-repeat fails for object iterations with identical values #1389 and this fixed version of the example fiddle: http://jsfiddle.net/nMjet/

Tests

I have added a new test to ng-repeatSpec.js to test for this but x-ed out the test that demonstrates the incorrect behaviour.

Algorithm

The algorithm checks the following, for original collection and changed collection, at each index.

1. If changed[index] === original[index] then simply reuse element/scope pairing.
2. If both changed[index] and original[index] are primitives then leave the element alone but update the scope model. This will trigger changes in the this iteration's template bindings and DOM. We can't try to guess if this primitive moved from elsewhere but since we are reusing the element/scope pairing there will be minimal DOM updates.
3. If a known object has moved then move its element and scope accordingly. This reuse ensures minimal disruption to the bindings and DOM.
4. if an object moved leaving a primitive in its place then create a new element and scope for this primitive.
5. if an object moved into a position where a primitive was before, then destroy the old element/scope for that primitive.

WhatChanged service

I factored out the algorithm that works out what has changed into its own function (whatChanged). Since there is potential for this to be reused, I also exposed it as a service, $whatChanged, in the ng module.

Helper Classes

Further to facilitate repeating over object key, the whatChanged algorithm works on a "generic collection", which are created by the internal classes ArrayWrapper and ObjectWrapper. These classes expose a common API of get(index), key(index), length() and copy(), which allows the whatChanged algorithm to work either with a straight array or iterating over the keys of an object, cleanly. The array wrapper is very thin and should not be a great impact on performance.

These (and a couple of other helper classes ObjectTracker, ChangeTracker and FlattenedChanged) can be found in apis.js. These new classes replace the need for HashQueueMap, which could be removed.

To Do

There is work to be done on the documentation.

I have done no performance testing. There are more lines of code and probably in most scenarios the repeater may perform worse than before. But I reckon there is lots of room for optimization.

[petebacondarwin]

Fixes: #933:

http://jsfiddle.net/KtfY6/
http://jsfiddle.net/vWQ4Z/
http://jsfiddle.net/BTcNT/

[ggoodman]

@petebacondarwin amazing. I hope @IgorMinar and/or @mhevery get on board with this as refactoring out the $whatChanged service will be very helpful for directives to act on changes to an underlying object/array without needing to create useless DOM in an ng-repeat to fake this behaviour.

+1

[diseaz]

Works wrong with keys when elements removed from an array:

<div ng-repeat="(k, v) in mylist">
  <div class="input-prepend">
    <button class="btn" ng-click="RemoveElement(k)">X</button>
    <input type="text" ng-model="mylist[k]"/>
  </div>
</div>

$scope.RemoveElement = function(idx) {
  $scope.mylist.splice(idx, 1);
};

Fix: https://github.com/diseaz/angular.js/commit/83da0020673ebfb93ed032fae651131f17dc2eab

[petebacondarwin]

@diseaz Thanks for this catch. Could you provide a working Plunker or unit test that demonstrates this issue? I am not sure I follow exactly what is the problem.

[NickHeiner]

I cloned and built this locally, and it fixed issues I was having related to #1267. Awesome!

[diseaz]

@petebacondarwin error demo: http://embed.plnkr.co/gZy4HT/preview
fix demo: http://embed.plnkr.co/4uEAB0/preview

Try removing elements and look at keys and $indexes (especially at the last one).

[petebacondarwin]

@diseaz. Thanks for looking into this and providing a very comprehensive demo. It is great to have more eyes on such a major reimplementation.

So what you are doing is using the syntax for iterating over an object's fields (i.e. "(key, value) in object)" but using it on an array. This is confusing for the ng-repeat because if you are removing fields from an object then the keys do not change but if you remove an item from an array the indexes of the items will change.

If you are iterating over an array you should use the other syntax (i.e. "value in array") and then use the $index property on the scope if you need to know an items "key". Is there some reason why you can't use this syntax?

I am pretty sure that what you are doing is not and should not be supported. @mhevery & @IgorMinar : Can you confirm if this is the case? If it is not supported then I would suggest that we thrown an exception if an array is passed as source but with the iterate over object syntax.

While this change does not break the current unit tests it will slow down ng-repeat updates where objects are simply being moved around an array. I would be interested to here from the core team on this before adding this change.

Also, @diseaz, we would need some unit tests that demonstrate the "bug" before the "fix" could be included.

[diseaz]

@petebacondarwin I've committed unittests into the same branch: https://github.com/diseaz/angular.js/tree/t-repeat

Everyone who writes in Javascript knows

for (var i in collection) {
  doSomethingWith(collection[i]);
}

works absolutely the same whether collection is an array or an object. Why ngRepeat should break this expected similarity? And even more: breaking support for keys while iterating arrays will be a regression. 1.x.y vesions support this behavior, and my unit tests pass on master and v1.0.x branches.

[NickHeiner]

There are other places where javascript syntax needs to be altered slightly to work as a directive argument. For instance, === isn't allowed (I'm pretty sure). Those alterations can be a gotcha but in general don't really bother me.

[petebacondarwin]

@diseaz : http://stackoverflow.com/questions/500504/javascript-for-in-with-arrays

[diseaz]

@petebacondarwin it's about performance, not about logical difference.

[petebacondarwin]

Hi Dmitry

I am not sure what you mean by performance over logic. The thing is that objects and arrays do behave differently when you manipulate them, which is where the issue you raise lies, and what I was getting at with the reference to the Stack Overflow question.

For a start, even in your example, you can't treat the dictionary and the array the same, you have different functions for removing items, since objects don't have splice. If you were consistent then you would use delete on both and of course this doesn't work as expected: http://plnkr.co/edit/ZCNkyL?p=preview.

Also, adding items is to an array is different from adding items to an object. Take this example using AngularJS 1.1.1: http://plnkr.co/edit/dURr0qrQSCTjjNActj8F?p=preview.

Pete

[diseaz]

@petebacondarwin yes, arrays and objects behave differently, but iteration has the same wording. ngRepeat is about iteration, not other behaviors. If key-value iteration for arrays is to be decided illegal — Ok, I can live with that. But I don't think a couple of lines of code is a big price for the feature that is natural for JS and already exists (and works!) in previous versions.

You need a fix anyway, because $index is not handled properly either. Though it could be a minor defect for iteration over objects, it's vital for arrays — now there is no correct index while iterating.

[petebacondarwin]

@diseaz :
You are right! The last $index was always wrong when deleting items in the (key,value) case. Thanks!

I spoke with Misko and he said that, while we are supporting iterating over keys, we should also support it over arrays. I will include your change but I suggest the following slight tweak so that we are not unnecessarily updating the DOM if we are just iterating without keys:

...
if ( item.moved ) {
  // An object has moved here from somewhere else - move the element accordingly
  newChildItem = originalChildItems[item.oldIndex];
  if ( keyIdentifier && isArray(source) ) {
    updateScope(newChildItem.scope, item.value, item.index);
  }
  newChildItems.push(newChildItem);
  currentElement.after(newChildItem.element);
...

If you want to add this and send me a PR then I'll push your commits into my PR.

[coli]

Just curious, does this work if the ng-repeat is used with filter?
eg

<ng-repeat = " primitive in array | filter:func" >

[petebacondarwin]

@coli: http://plnkr.co/edit/DkSJb4?p=preview

[petebacondarwin]

@diseaz : Landed your changes as: petebacondarwin@ee75ccf

Thanks for bearing with me. Do post if you find any more issues.

[diseaz]

@petebacondarwin thanks a lot!

[IgorMinar]

ok, so I started revewing this PR but after a while I came to realization that it adds too much overhead.

the problems it solves are good problems and the solution is quite good too, but given that ngRepeat is one of the most used directives and its already one of the most expensive directives, this change is just too heavy weight for us mainly because the repeater is trying to be too generic and solve all kinds of problems.

There is a good news though. What if we solved all the problems in a different, less generic, which also means more lightweight way? Here is how we could do it:

• currently the repeater is stable by default (creates 1:1 identity based association between objects and DOM): this is great in certain cases, but in most scenarios this only adds overhead. So what if we made it unstable (1:1 association between the repeater index and DOM) by default.
• if the developer does require the repeater to be stable, then he can tell us so explicitly (syntax TBD)
• the stable repeater will require all keys in the repeater to be unique. which means that it will work with both objects and primitives, but the primitive keys are expected not to be mutable (if this is not what you want, you want to use unstable repeater instead).

how does this sound to you?

[mhevery]

Here is my syntax proposal.

<div ng-repeat="item in items"></div>

This is what we would call unstable. Or to put it better, it is stable with respect to the position of the item in the array. So we could rewrite this as:

<div ng-repeat="$index from item in items"></div>

(The exact syntax is up for discussion)

The above means that the DOM elements are associated with the $index and that we use the item property on the child scope.

<div ng-repeat="$hash(item) from item in items"></div>

The above would be equivalent to the current behavior since the $hash function would decorate the object with the $$hashKey and it would also return it. This would create the uniqueness ID for each item and the ID would then be associated with the DOM.

<div ng-repeat="item.id from item in items"></div>

The above would allow the users to use properties on existing objects such as DB IDs as the DOM association keys.

or we could put the hash function on the model

<div ng-repeat="item.myHashFn() from item in items"></div>

or even on the controller

<div ng-repeat="myHash(item) from item in items"></div>

[coli]

Oh yes, allow the user to specify the key to use for the change detection is a great idea, solves the server side refresh of item list problem well. Thanks for coming to your senses :)

How about when iterating over the object fields and using the field name as the key, what would the syntax look like?

[petebacondarwin]

I like @mhevery's suggested syntax, it is powerful and seems fairly intuitive.

Would you propose to still support object property iteration? I guess the syntax would be

<div ng-repeat="(key, value) in someObject"></div>

to keep it backwardly compatible. Under the covers this is really equivalent to:

<div ng-repeat="key from (key, value) in someObject"></div>

Making it a stable repeater?

The breaking change to people moving from the current sitation to using the unstable repeater (if they don't change their usage), is just that their DOM elements will be rebound to a new scope whenever they reordered their collection rather than being reused and moved around? Would this have a fundamental impact in how it works or just a performance degradation?

As an aside, I think that an implementation of this would require a modification to the $watch process in line with #1093.

[mhevery]

While this is a breaking change, I think unless you are animating the movements of the elements in the DOM, it should have very minimal impact on the developer.

[ggoodman]

@mhevery @petebacondarwin couldn't you avoid making this a breaking change by defaulting defaulting to a stable iteration using the (newly made public) angular.identity() function.

Can you articulate why it would be a good idea to default to unstable versus defaulting to stable?

I think you may underestimate how much code might rely on the stable iteration (at least in my codebases). Wouldn't it be safer to make the unstable iteration opt-in instead of opt-out?

Another syntax idea that may be clearer (though perhaps less consistent with normal array comprehension syntax):

<div ng-repeat="(key, val) in object using key.id"></div>

Just a quick alternative idea.

[IgorMinar]

@ggoodman can you provide some specific examples where you depend on identity based repeaters?

[IgorMinar]

@ggoodman I was looking at LINQ query comprehension syntax yesterday which is left-to-right similar to your example. I like that syntax better. http://rthumati.wordpress.com/category/net/02-data-access/linq/ (Scroll down to 2/3 of the page)

[ggoodman]

Hi @IgorMinar I think that I may have misunderstood the issue.

Can you confirm that the breaking change will only affect arrays/hashs wherein the values are primitives? (Number, String, Boolean)?

If that is the case, I do not understand where the breakage is.

Now, if the new behaviour would default to using the index/key of items even if the value is an object derivative (and could thus have an $$oid), then I would have a problem.

The issue would arise from the fact that I often use ng-repeat directives on hidden divs to manage sub-directives for arbitrary length sub-components. An example would be the edit area component of Plunker that can have one or more attached buffers. Those buffers are instantiated as a custom directive that is ng-repeated (example).

If ng-repeat started creating/destroying buffers as their position in the hash changed it would mean huge slowdowns and loss of undo history/state for all of those sub-directives.

[petebacondarwin]

I think the names instable and stable are misleading. Instead we should
refer to index-based and key-based repeaters.

The default would be index-based - which means that each repeated element
in the DOM is associated with an index in the array - so if you decided to
reorder the array items the DOM elements would not be destroyed but rebound
to different values from the same index position in the array. This is how
it has to work with primitives anyway since there is no way of tracking
them, from an identity point of view. This would have the same syntax as
the current situation and from a programmer's point of view there would be
little change other than a potential performance hit (as Geoff mentions)
and problems if you were currently expecting your elements to move around
with your array items (as mentions by Misko).

The other option would be a key-based repeater, where each DOM element is
associated with an immutable key that uniquely identifies an item in the
array. The key would be specified by the syntax of the ng-repeat. It could
be a property or a method (to be called) on the array item, if it were an
object, or it could be an external function that takes the item as a
parameter. In any case because of the immutability of the keys, the
repeater could than optimize the association with the DOM elements so that
it could reuse the elements if the array items were only moved around
within the array.

@geoff - I think this is an acceptable change. If the developer upgrades
and does nothing to change their repeaters, they would still work in almost
all cases with the only downside being a potential perfomance degradation.
If this is a problem then it would not be a big deal to modify poorly
performing repeaters to use the key-based repeater. It would almost be a
simple regex find and replace in most cases.

On 5 February 2013 18:33, ggoodman [email protected] wrote:

Hi @IgorMinar https://github.com/IgorMinar I think that I may have
misunderstood the issue.

Can you confirm that the breaking change will only affect arrays/hashs
wherein the values are primitives? (Number, String, Boolean)?

If that is the case, I do not understand where the breakage is.

Now, if the new behaviour would default to using the index/key of items even
if the value is an object derivative (and could thus have an $$oid),
then I would have a problem.

The issue would arise from the fact that I often use ng-repeat directives
on hidden divs to manage sub-directives for arbitrary length
sub-components. An example would be the edit area component of Plunker that
can have one or more attached buffers. Those buffers are instantiated as a
custom directive that is ng-repeated (examplehttps://github.com/filearts/plunker_www/blob/master/assets/js/directives/ace.coffee#L156-L157
).

If ng-repeat started creating/destroying buffers as their position in the
hash changed it would mean huge slowdowns and loss of undo history/state
for all of those sub-directives.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1661#issuecomment-13143967.

[petebacondarwin]

Moreover, @IgorMinar, now that you mentioned Linq, I was thinking that it
would be good to support some kind of enumeration interface that allowed
people to chain transformations onto enumerable arrays. This would allow
array filters to be developed more easily without inadvertently creating
non-idempotent watchers, and instable $digests.

On 5 February 2013 19:01, Peter Bacon Darwin [email protected] wrote:

I think the names instable and stable are misleading. Instead we should
refer to index-based and key-based repeaters.

The default would be index-based - which means that each repeated element
in the DOM is associated with an index in the array - so if you decided to
reorder the array items the DOM elements would not be destroyed but rebound
to different values from the same index position in the array. This is how
it has to work with primitives anyway since there is no way of tracking
them, from an identity point of view. This would have the same syntax as
the current situation and from a programmer's point of view there would be
little change other than a potential performance hit (as Geoff mentions)
and problems if you were currently expecting your elements to move around
with your array items (as mentions by Misko).

The other option would be a key-based repeater, where each DOM element is
associated with an immutable key that uniquely identifies an item in the
array. The key would be specified by the syntax of the ng-repeat. It could
be a property or a method (to be called) on the array item, if it were an
object, or it could be an external function that takes the item as a
parameter. In any case because of the immutability of the keys, the
repeater could than optimize the association with the DOM elements so that
it could reuse the elements if the array items were only moved around
within the array.

@geoff - I think this is an acceptable change. If the developer upgrades
and does nothing to change their repeaters, they would still work in almost
all cases with the only downside being a potential perfomance degradation.
If this is a problem then it would not be a big deal to modify poorly
performing repeaters to use the key-based repeater. It would almost be a
simple regex find and replace in most cases.

On 5 February 2013 18:33, ggoodman [email protected] wrote:

Hi @IgorMinar https://github.com/IgorMinar I think that I may have
misunderstood the issue.

Can you confirm that the breaking change will only affect arrays/hashs
wherein the values are primitives? (Number, String, Boolean)?

If that is the case, I do not understand where the breakage is.

Now, if the new behaviour would default to using the index/key of items even
if the value is an object derivative (and could thus have an $$oid),
then I would have a problem.

The issue would arise from the fact that I often use ng-repeatdirectives on hidden divs to manage sub-directives for arbitrary length
sub-components. An example would be the edit area component of Plunker that
can have one or more attached buffers. Those buffers are instantiated as a
custom directive that is ng-repeated (examplehttps://github.com/filearts/plunker_www/blob/master/assets/js/directives/ace.coffee#L156-L157
).

If ng-repeat started creating/destroying buffers as their position in
the hash changed it would mean huge slowdowns and loss of undo
history/state for all of those sub-directives.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1661#issuecomment-13143967.

[ggoodman]

@petebacondarwin can you elaborate on what the default behaviour would be for enumerating over hashes? Would the new system preserve key => DOM mapping as was the case previously?

[petebacondarwin]

I would assume that repeating over an object's properties would be a
"key-based repeater" since you already have an obvious key available.
Therefore you would indeed have the key<=>DOM association still in place.

On 5 February 2013 20:02, ggoodman [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin can you elaborate
on what the default behaviour would be for enumerating over hashes? Would
the new system preserve key => DOM mapping as was the case previously?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1661#issuecomment-13148808.

[mb21]

as far as I can tell, the issue of the input losing focus hasn't been entirely fixed in angular 1.1.2 http://jsfiddle.net/pUuC5/5/
here's a workaround: http://jsfiddle.net/pUuC5/7/ (hat tip to http://stackoverflow.com/questions/11868393)

[IAmBrandonMcGregor]

I agree with @mb21. On both 1.0.5 and 1.1.3, I am having this issue still.

[klebba]

This is definitely a useful option/addition. Is it going to make it in soon?

[VELIKII-DIVAN]

temporary solution for 1.0.4, 1.0.3: http://jsfiddle.net/5AAZz/

[IAmBrandonMcGregor]

@VELIKII-DIVAN Awesome! Much better work around than the previously suggested directive. This one live updates view bindings rather than waiting for the blur event. Thanks!

[sandipchitale]

@VELIKII-DIVAN Awesome indeed!

[petebacondarwin]

This is going to be solved a different way so closing this for now.

[e-oz]

I can't find in documentation how I can bind ng-repeat to string. Sometimes it can be useful. List of tags, for example.

[thatmarvin]

@petebacondarwin Is there an issue that tracks this different way?

[petebacondarwin]

ng-repeat="item in listOfStrings track by $index"?

On 6 September 2013 21:47, Marvin Tam [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin Is there an issue
that tracks this different way?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1661#issuecomment-23968206
.

[thatmarvin]

Ah ok, didn't realize track by was meant to address this, thanks @petebacondarwin !