angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.83k stars 27.51k forks source link

ng-repeat Track By evaluates only on Array length or reference change. #15785

Closed thuelsmeier closed 7 years ago

thuelsmeier commented 7 years ago

Do you want to request a feature or report a bug? bug

What is the current behavior? Object properties in track by evaluates only on the change of the Array length or reference.

The steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co

  1. http://plnkr.co/edit/VOlmRHH0NnsaAfarR0hd
  2. Click on "changeName"-button -> You will notice no changes and no console output
  3. Click on "addItem"-button -> Now you will notice that the first item is rerendered and console logs access to all track by object properties

What is the expected behavior? This behavior seems to be inconsistent. One of the following is expected, but not mix of them:

  1. object properties in track by should get evaluated on every digest like other components like ng-model and lead to the removal of the old DOM and adding of new DOM on change.
  2. only changes or new references should get evaluated.

First behavior is more preferable, because this introduces consistency with other angular mechanics.

Is there a possible workaround for this inconsistency to get the first behavior?

Narretz commented 7 years ago

I believe this works as expected, as this happens only when you use one-time binding in the ngRepeat template. It's also documented that ngRepeat uses $watchCollection to watch the collection, i.e. it checks the reference of the items, but not the content of the items.

When you update the name property in items[0], the template will not be updated because the property is one-time bound and ngRepeat will not call the tracking function because no item reference in the collection changed.

When you add an item, ngRepeat must rebuild the rendered collection, and notices that item[0] has a new tracking id, which causes the template to be re-built, which recreates one-time binding.

So in conclusion you shouldn't use one-time binding if you plan to change the contens of your items dynamically.

Bessonov commented 7 years ago

I agree with @IceFight. Technically, It's OK to use $watchCollection to watch the collection. But for track by documentation says:

If you are working with objects that have a unique identifier property, you should track by this identifier instead of the object instance.

If this unique identifier property changes, then it is expected that collection elements are not equals anymore. Think of default behavior of two-way-binding.

Furthermore, depending on phase of ngRepeat, current behavior is like between (pay attention to :: after track by): <li ng-repeat="item in items track by item.id()">{{::item.name}}</li> and <li ng-repeat="item in items track by ::item.id()">{{::item.name}}</li>.

Narretz commented 7 years ago

I think the description of track by is incorrect / misleading in this case. It only tracks the item's position in the rendered output with the trackBy function. That means, when ngRepeat notices a change in the collection, it goes through all items and calls their trackBy Fn. If ngRepeat finds a corresponding DOM element, it will update this element, but not re-create, for better render performance. If you don't have a track by Fn, ngRepeat will use a hash to detect if the object already exists (the hash is based on object identity). When the objects in the collection get replaced, this means ngRepeat has to re-render, because new objects mean new hashes. When the objects get replaced, but their track value is the same, ngRepeat can maintain the rendered output.

If ngRepeat would watch the trackBy Fn for changes, this would actually be much more expensive than watchCollection.

So I think we just need to make this clearer in the docs.

Bessonov commented 7 years ago

@Narretz you describe current technical behavior. As I understand, the question is of expected behavior. I think argue with technical behavior is not right thing.

If ngRepeat would watch the trackBy Fn for changes, this would actually be much more expensive than watchCollection.

I don't think so. You can apply several cache/invalidation technics, or just use... property, which you can set. Think of <li ng-repeat="item in items track by item.lastChangedDate">{{::item.name}}</li>. Just simple reference comparison. But with huge benefit: you can use one-time-binding for whole inner block! That's an expressive performance boost! Is ngRepeat not a reason for what one-time-binding was invented for in first place?

Narretz commented 7 years ago

I am not just talking about the implementation, I am talking about the intention behind the feature. It's possible that you expect something different from track by based on description or your use case.

I'm not 100% sure what you propose there but changing the collection change detection strategy is something that is independent from the track by mechanism. It would have to be incorporated into the watch mechanism.

Narretz commented 7 years ago

I'm going to close this issue because we haven't got any (further) feedback. Feel free to reopen this issue if you can provide new feedback. I've re-read the ngRepeat docs, and I believe tracking is sufficiently explained as being a different operation than watching the underlying collection for changes.

Generic proposals for the $watch functionality: https://github.com/angular/angular.js/issues/10069, https://github.com/angular/angular.js/issues/11177

thSteve commented 6 years ago

Our web app uses ngRepeat to display a list of items. The array and its objects are never changed, but values of the objects inside can be modified by the user. These models are stored in a service and can be accessed elsewhere in the app at the same time.

We are using the one-time binding syntax inside each item to reduce the number of watchers on the page, as it can quickly climb to the thousands, since each row can have ~100 bindings on it. Without one-time binding and "track by", the page is unusably slow.

We generate a unique trackId for each item. This trackId is updated every time the item's values change. However, when the trackId changes, the items are not rebuilt.

Here's a bare-bones example. The track ID changes but the item is not rebuilt. https://plnkr.co/edit/Lklq3ZNDUuggjgwmkoxj?p=preview

From the docs:

Custom Expression: It is possible to use any AngularJS expression to compute the tracking id, for example with a function, or using a property on the collection items. item in items track by item.id is a typical pattern when the items have a unique identifier, e.g. database id. In this case the object identity does not matter. Two objects are considered equivalent as long as their id property is same. Tracking by unique identifier is the most performant way and should be used whenever possible.

This implies that if the trackId's in the collection are to change, they would no longer be considered equal and would thus be re-evaluated. So by definition, the items inside of the ngRepeat are now different and yet the view is not updated to reflect that.

gkalpak commented 6 years ago

This happens, because ngRepeat will essentially use $watchCollection('friends', ...) under the hood and only apply the track by logic if the collection has changed. This means that track by will only work if the collection changes (and will be able to map a new object to an old one). You can also see that all entries are correctly updated when adding a new item to the collection.

Theoretically, we could watch the results of track by for each item, but that be a little expensive (which is a concern for such a commonly used directive such as ngRepeat).

As a workaround, you can use "immutable" objects (i.e. ensure that you create a new instance whenever you want to update a property, instead of updating it in-place).

jbedard commented 6 years ago

@thSteve the way you are using track-by is essentially the opposite of what it was intended for, although you do highlight an interesting use case/bug. But I'm pretty sure fixing it would cause too many issues (digest performance) for the normal use case.

As @gkalpak noted one solution would be using immutable objects and removing the use of track by. Then using one-time like you are now to avoid many watchers within the ng-repeat.

Another solution we thought is using $scope.$suspend() (new in 1.6.10: https://github.com/angular/angular.js/commit/41d5c90f170cc054b0f8f88220c22ef1ef6cc0a6) instead of track by + one-time bindings. I think this would be the best option for performance, as long as it's ok to make every expression within the ng-repeat "one-time". Essentially:

Might be a little tricky getting a reference to that suspended child scope in the parent scope. Might be best with a help directive (or two? a child one that injects its scope into the parent one?). But I'm pretty sure it's possible...