Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Add cache option to ngRepeat to keep filtered out DOM element #1865

Closed
wants to merge 1 commit into from

Conversation

jmaynier
Copy link

Add keepCache parameter to have the option to keep in a cache and hide DOM elements associated to filtered out object of the collection. Make it faster to display again those element when filtered in.

For a large collection, it is sometimes very slow for angular to recreate all DOM node that were filtered out and are now filtered in. This prevent the deletion of those DOM nodes and keep them in a cache.

Usage:

… a cache and hide DOM elements associated to filtered out object of the collection. Make it faster to display again those element when filtered in.
@petebacondarwin
Copy link
Contributor

Could you not simply remove the filter altogether and add a ng-hide directive to your items that will be repeated?

<div ng-repeat="x in items" ng-hide="filterOut(x, $index)>{{x}}</div>

where filterOut would return true if this item is to be hidden.

@jmaynier
Copy link
Author

Thanks Peter for the suggestion.

The thing is my items collection is coming from a service and used in several places. It is filtered in the service for that reason (I used to filter in the template, but I had to do it at several places so it was not DRY nor efficient).
For example I compute aggregated data based on the filtered results.

Here is a simplified real example (without the keepCache patch): http://plnkr.co/edit/APkF1r1aM3NNHx9R8S6e?p=preview

With your solution, it means I need to maintain a collection unfiltered to put in this ngRepeat, and another one filtered for other uses, and call the filtering function twice (in the service for the filtered collection + in the ngHide on the ngRepeart node, the later could be call several time per $digest)

Something like : http://plnkr.co/edit/3tUfHwvGiQMp6pdDou8D?p=preview

I will try your suggestion in my real app (where creating the ng-repeat node is quite expensive), but it will still be less efficient.

@petebacondarwin
Copy link
Contributor

@jmaynier
I appreciate that my suggestion is far from ideal. What sort of size are we talking about that are slow to update? What is being bound and how are you setting up the bindings? You could have a go at trying my alternative ng-repeat (#1661) since it has a subtly different algorithm for dealing with changes to the list.

My main concern with this change is that it would lead to potential memory leaks. How would one clear the cache at a later stage?

@jmaynier
Copy link
Author

@petebacondarwin

I tried in my real app your suggestion (based on the idea demoed in http://plnkr.co/edit/3tUfHwvGiQMp6pdDou8D?p=preview). I used the unfiltered list in the ngRepeat with ng-hide + filter and the filtered one everywhere else.
As you said it has the advantages to not leaking memory. The number of DOM elements are the size of the collection.

Their is no perceptible performance difference between my hack on ngRepeat and your solution (around 1-2 seconds to filter/display list, more than 5 seconds with the raw ngRepeat on the filtered list).

So I can stay with that solution and close the PR. What will be perfect would be a virtual scroll solution for ngRepeat (using ng-grid is not ideal as I want to display a feed, with 2 ngRepeat nested, not data in row). So memory print would be lower and time to display faster. Do you know if it will be build into ngRepear in the near future ?

@petebacondarwin
Copy link
Contributor

Thanks for coming back on this. I will close for now.
Regarding virtual scrolling. I don't think this will be in the core in the near future. Perhaps you could look at how they did it in ng-grid and use the same technique for your own directive?

@jmaynier
Copy link
Author

Thks again Peter for your help. I will see how I can integrate virtual scrolling in a fork of ngRepeat.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants