Value checking in ngModel length watch?

[fishfacemcgee]

I'm using ui-sortable in a web app that can contain a very large number of elements to sort. I've noticed that initial load can be extremely long, especially in Firefox where the "Unresponsive Script Warning" ends up showing up between 3 and 4 times before everything is finalized. I understand this is a bit of an extreme case. However, I noticed that if I make a very small adjustment to the attrs.ngModel+'.length' watch, everything loads faster.

Before:

 72             // When we add or remove elements, we need the sortable to 'refresh'
 73             // so it can find the new/removed elements.
 74             scope.$watch(attrs.ngModel+'.length', function() {
 75               // Timeout to let ng-repeat modify the DOM
 76               $timeout(function() {
 77                 // ensure that the jquery-ui-sortable widget instance
 78                 // is still bound to the directive's element
 79                 if (!!element.data('ui-sortable')) {
 80                   element.sortable('refresh');
 81                 }
 82               });
 83             });

After

 72             // When we add or remove elements, we need the sortable to 'refresh'
 73             // so it can find the new/removed elements.
 74             scope.$watch(attrs.ngModel+'.length', function(newVal, oldVal) {
 75               if (newVal === oldVal) {
 76                 return;
 77               }
 78               // Timeout to let ng-repeat modify the DOM
 79               $timeout(function() {
 80                 // ensure that the jquery-ui-sortable widget instance
 81                 // is still bound to the directive's element
 82                 if (!!element.data('ui-sortable')) {
 83                   element.sortable('refresh');
 84                 }
 85               });
 86             });

As you can see, the only thing I changed was adding newVal/oldVal parameters and checking to make sure something changed before doing anything else.

The main reason why I don't have this as a Pull Request is I'm fairly ignorant as to what the consequences of this change, if any, are. So far, everything tied to drag and drop seems to perform just fine, so I think it's worth adding.

[thgreasi]

Actually, this is a piece of code that I wanted to review for some time.
First of all, this should be a watchCollection instead of a plain watch, but this would drop angularjs v1.0 support. I've been thinking of creating and using a polyfill for that reason, but never got some time on it.
When you add a watch, it instantly runs one time and reports oldVal === newVal. This is by design and most of the times reduces the code duplication for calculated values and since we have a way to detect if it's the initial run, we can skip it. As a result, if you add a watch on a scope variable, and then change it 10 times, the code of the watch will be executed 11 times.

In our case now.
The sortable element always gets initialized with no elements in it, since the ng-repeat has not yet created the DOM elements for the items.
After this, each sortable option is applied.
Finally after everything else has finished, the sortable gets refreshed so that it detects the items produced by the ng-repeat.

I will run some tests with your proposal and possibly create a new branch so that you can also test it on your code and provide some feedback.

[fishfacemcgee]

@thgreasi Sounds great. Thanks for the insight into how this part of the library works, too.

[thgreasi]

All the tests seem to pass with your change!
But, it still feels required to me to do the first refresh, since the ng-repeat generates the DOM elements after the sortable gets initialized.

[fishfacemcgee]

Hm. I may be avoiding that issue in my use-case as I'm lazy-adding the models to the collection used in ng-repeat due to the high-element count. So, most likely in the true initial state (in my scenario), newValue may simply be null (which may be a better thing to check for) before the lazy-add adds additional models.

I'll try out the null check in my setup and see if solves the problem as well, as that may be a more sane thing to prevent.

[fishfacemcgee]

Unfortunately, the null check wasn't sufficient to prevent the performance issue. As long as you don't think there's some terrible consequence to doing the newVal/oldVal check (at least in the context of sortable still functioning), I'll just keep the change in my local copy.

Thanks for taking a look at this.

[thgreasi]

#292 should improve/solve your problem. Can you give it a try?

[fishfacemcgee]

@thgreasi If I remove the newVal/oldVal checking and make the changes in #292, the performance issues return, unfortunately.

[thgreasi]

What if you change the $timeout with a plain timeout?

[fishfacemcgee]

@thgreasi No dice.