ng-select/ng-options causes underlying data to be changed, when assiging properties on ng-model

[russbuelt]

Hello!

we encountered some odd behaviour, when using ng-select and changing the ng-model not through a selection.

http://jsfiddle.net/r5xanz0s/1/

There are 2 different scenarios (starting after a reload):

1. When assigning new properties for ng-model (via "Assign 'someName3' properties only") the ng-select is not updated at all.
2. When selecting 'someName2' and then assigning new properties for ng-model (via "Assign 'someName3' properties only") the entry for 'someName2' within the select is changed to data from 'someName3'. This results in a duplicate entry.

This behaviour does not occur, when assigning a new object to ng-model.

Any ideas?

Best,
Gregor

[vitaly-t]

Looking at the fiddle, I'd like to take a guess here, based on what I see...

When a selection changes, "selected" seems to be assigned the corresponding "option" object, i.e. as a reference instead of deep copy, which explains the behaviour then - changing such "select" properties after that will change the option that was last selected.

And if I am right, then switching to deep copy for the "selected" property should fix the issue. And I would watch out for other issues that may be triggered by the change, because it appears to be a fundamental issue. It needs some good testing.

[vitaly-t]

I still hope one of the experts will respond to it soon, as this is a serious issue and a 100% bug.

[frfancha]

I looked at the fiddle, and I don't see any bug here. What is wrong and what should be the correct behavior instead of the wrong part for you?

[pkozlowski-opensource]

I can see what the opener is complaining about - clicking "Assign 'someName3' properties only" should select someName3 - I can reproduce this in Chrome with the latest version on the first load. BUT https://docs.angularjs.org/api/ng/directive/ngOptions is saying:

Do not use select as and track by in the same expression. They are not designed to work together

which makes sense. Unfortunatelly correcting the ngOptions expression doesn't make things work properly and the bug is present in the latest version as well (http://jsfiddle.net/bysb3tt9/). Interestingly this pb happens only on the first load - selecting option 2 before clicking buttons makes the pb go away.

@petebacondarwin hopefully you still remember select / ngOptions code from your latests refactoring efforts :-)

[petebacondarwin]

I'll have a look tomorrow

[vitaly-t]

@petebacondarwin

Judging by just the recent bugs logged against select/drop-box, it is the most flaky component in the entire library. Could we, please get some serious attention to the element support and rework it into something more reliable?

Other most recent issues:

#10887
#10782

[russbuelt]

@pkozlowski-opensource thanks for pointing out the hint within the documentation - i have clearly missed it.

The most confusing part for us is the reference which is assigned to ng-model; we would expect to have a copy.

[pkozlowski-opensource]

@russbuelt nope - it is not a copy but rather the exact reference (for objects)

[petebacondarwin]

This issue is beset by the problem of ngModel expecting models to be atomic things (primitives/objects).

When it was first invented it was expected that ngModel would only be a primitive, e.g. a string or a number. Later when things like ngList and ngOptions were added or became more complex then various hacks were put in place to make it look like it worked well with those but it doesn't.

---

Just to be clear what is happening in this issue, lets name the objects:

var option1 = { uid: 1, name: 'someName1' };
var option2 = { uid: 2, name: 'someName2' };
var option3 = { uid: 3, name: 'someName3' };

var initialItem = { uid: 1, name: 'someName1' };

model {
  options: [option1, option2, option3],
  selected: initialItem
};

Now when the fiddle begins we have:

expect(model.selected).toBe(initialItem);
expect(model.selected.uid).toEqual(option1.uid);
expect(model.selected).not.toBe(option1);

So although ngOptions has found a match between an option and the modelValue, these are not the same object.

Now if we change the properties of the model.selected object, we are effectively changing the initialItem object.

model.selected.uid = 3;
model.selected.name = 'someName3';

expect(model.selected).toBe(initialItem);
expect(model.selected.uid).toEqual(option3.uid);
expect(model.selected).not.toBe(option3);

At the moment ngModel only watches for changes to the object identity and so it doesn't trigger an update to the ngOptions directive. We can fix this in ngOptions by adding a deep watch on the attr.ngModel expression...

scope.$watch(attr.ngModel, updateOptions, true);

I have done that in this Plunker: http://plnkr.co/edit/0PE7qN5FXIA23y4RwyN0?p=preview

We can and should do this, although we should probably only do it if we have a track by expression.

---

But this isn't the end of the story. Since ngModel and ngOptions do not make copies between the model and the view, we can't go around just changing the properties of the model.selected object. This is particularly important in the situation where the user has actually chosen an option, since the model.selected will now points directly to one of the option objects:

// User selects "someName2" option
expect(model.selected).toBe(option2);
expect(model.selected.uid).toEqual(option2.uid);
expect(model.selected).not.toBe(initialOption);

If we now change the model.selected object's properties we are actually changing the option2 object:

expect(model.selected).toBe(option2);

model.selected.uid = 3;
model.selected.name = 'someName3';

expect(model.selected).toBe(option2);
expect(model.selected).not.toBe(option3);

expect(option2.uid).toEqual(3);
expect(option2.name).toEqual('someName3');

Which means that the options are now broken:

```js
expect(model.options).toEqual([
  { uid: 1, name: 'someName1' },
  { uid: 3, name: 'someName3' },
  { uid: 3, name: 'someName3' }
]);

We should be able to fix this if the ngOptions makes copies when reading the value if track by is being used. If we are not using track by then we really do care about the identity of the object and should not be copying...

      selectCtrl.readValue = function readNgOptionsValue() {

        var selectedOption = options.selectValueMap[selectElement.val()];

        if (selectedOption) {
          removeEmptyOption();
          removeUnknownOption();
          return options.trackBy ? angular.copy(selectedOption.viewValue) : selectedOption.viewValue;
        }
        return null;
      };

I have done this in this Plunker here: http://plnkr.co/edit/YEzEf4dxHTnoW5pbeJDp?p=preview

I think this solves the problems in this issue. Anyone see anything else?

[petebacondarwin]

Actually, it is probably enough to call $render when the model changes...

scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true);

See http://plnkr.co/edit/kHAxSpodz3fSzjP7IlV2?p=preview

[petebacondarwin]

I have offered a fix - can someone review this? @pkozlowski-opensource maybe?

[pkozlowski-opensource]

@petebacondarwin I will have a look at it later in the evening if no one does so before.

[booleanbetrayal]

@petebacondarwin - this deep copy is slaughtering our performance. In our case, we're not using trackBy but we're still suffering the deep copy hit. Seems unnecessary for any situation where object reference equality is intended and suitable. Looking into a patch atm and may propose a PR, shortly.

[booleanbetrayal]

@petebacondarwin - Also, it appears that the documentation differs from this implementation:

According to select documentation:

Note: ngModel compares by reference, not value. This is important when binding to an array of objects. See an example in this jsfiddle.

[booleanbetrayal]

@pkozlowski-opensource ^

[petebacondarwin]

@booleanbetrayal could you open a new issue to track this?

[booleanbetrayal]

@petebacondarwin - Done. See #11447. Thanks!