Multiselect broken with Angular 1.3-rc.1

[mpolichette]

I'm sorry for the terse issue report I'll try and flush this out more later.

Has anyone else tried to use ui-select w/ angular 1.3? The multiselect has a weird behavior where you can add tags via mouse clicks but when you start typing any previously added are removed. The strangest bit is that if you've initialized it with an array of initial values, those are not removed.

It also doesn't appear to be updating ng-model.

This leads me to believe there is some sort of array caching problem.

If anyone has already found a solution i'd be happy to hear about it, otherwise I'll see if I can find some time to debug it later.

[mpolichette]

Also, you can observe the brokenness by replacing the plunkr script tags with the following:

<script src="http://ajax.googleapis.com/ajax/libs/angularjs/1.3.0-rc.1/angular.js"></script>
<script src="http://ajax.googleapis.com/ajax/libs/angularjs/1.3.0-rc.1/angular-sanitize.js"></script>

[dimirc]

Maybe could be related to #176. I think I only tested until 1.3-rc0 that was available when merged multi select PR

[mpolichette]

Its possible. I'm spending a bit of time today to debug. It looks like that parameter isn't even accepted for $setViewValue for my code... I'll follow up

[mpolichette]

So, I figured it out, it is a combination of array caching and data mutation.

The problem is a result of the following

1. When ctrl.multiple=true ui-select uses ctrl.selected.push(item); (here)
2. ui-select watches that array here and when it changes, it calls ngModel.$setViewValue with that array.
3. When angular gets around to fulfilling $setViewValue it has a perf check to prevent parsing/validation/etc.... if the view value has not changed. If it has not changed, it just returns early. If it has changed, it runs the parse/validation and commits it to the $modelValue.

This would normally be great. However! Because we mutated the $select.selected array and submitted it again, the angular checks (by reference) the submitted array against the previous one, and thinks, rightly that they're still the same.

This is a classic issue when doing data mutation :( My recommendation is to replace the array when updating. A simple way would be instead of
ctrl.selected.push(item); (here)
to do
ctrl.selected = ctrl.selected.concat([item]);

Thoughts?
I could throw together a pull request if thats an agreed solution.

[dimirc]

Yes, that perf check is the problem and on 1.3rc0 there was a third parameter on $setViewValue that allowed to skip that check.

I was thinking on trying to setViewValue to an empty array to make that perf check be false and then calling again setViewValue with correct value. Anyway about your proposal I think will be ok too and maybe cleaner. We should also keep on mind that a new array should returned when removing an item (currently using splice).

Also of we go ahead this way maybe watchCollection won't be needed but just a simple watch.

Will be good if you can go ahead with a PR and maybe some new test if there isn't one that is validating this.

Sent from my iPhone

On Sep 12, 2014, at 6:11 PM, Matt Polichette notifications@github.com wrote:

So, I figured it out, it is a combination of array caching and data mutation.

The problem is a result of the following

When ctrl.multiple=true ui-select uses ctrl.selected.push(item); (here)
ui-select watches that array here and when it changes, it calls ngModel.$setViewValue with that array.
When angular gets around to fulfilling $setViewValue it has a perf check to prevent parsing/validation/etc.... if the view value has not changed. If it has not changed, it just returns early. If it has changed, it runs the parse/validation and commits it to the $modelValue.
This would normally be great. However! Because we mutated the $select.selected array and submitted it again, the angular checks (by reference) the submitted array against the previous one, and thinks, rightly that they're still the same.

This is a classic issue when doing data mutation :( My recommendation is to replace the array when updating. A simple way would be instead of
ctrl.selected.push(item); (here)
to do
ctrl.selected = ctrl.selected.concat([item]);

Thoughts?
I could throw together a pull request if thats an agreed solution.

—
Reply to this email directly or view it on GitHub.

[dimirc]

@caitp when I opened angular/angular.js#8481 about using the third parameter you wrote:

In the future, most of this stuff will be moved out of $parsers/$formatters, and you should be able to just use the validate() api which is already exposed. Currently validate() isn't enough for this because the code which we need to revalidate lives in parsers/formatters, but it won't soon (I hope!)

Since this is related, do you have any better idea than the ones we are proposing on this issue, about how to get around the perf check that is causing us problems since 1.3-rc.1 (where third parameter doesn't exist anymore)

[caitp]

I believe we decided the viewValue should always be a string --- off the top of my head, you could look at serializing the array in some fashion, and deserializing it for the model value during parsing / rendering.

[dimirc]

@mpolichette I created PR #193 with how I was thinking on doing it. Works ok and maybe will be better to have less code changed, anyway let me know your comments.

[dimirc]

@caitp thanks for the confirmation. We'll be checking some alternatives based on this.

[mpolichette]

@dimirc Thanks for helping solve this.

While that solution in #193 will fix the primary issue, it has a side effect where it will run all the setViewValue code paths twice including parsing, validation, and calling any ng-change listeners with an empty string before calling w/ the correct value.

While thats not necessarily a terrible thing, it feels a bit like a hack solution to me. I'll try to get a PR going when I get a bit of spare time! :)