This repository was archived by the owner on Apr 12, 2024. It is now read-only.
fix(ngOptions): fix model<->option interaction when using track by #10893
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@petebacondarwin I don't know the code of select / ngOptions well enough to give you thumbs up but I would feel much better if tests could verify the impact of changes in the object on what is displayed by select (if possible). I guess this PR will need another pair of eyes or we can hangout together so you can walk me through the code / design. |
|
@pkozlowski-opensource - I updated the test. Let me know if this is still missing something... |
petebacondarwin
modified the milestones:
1.4.0-beta.3 / 1.3.12,
1.4.0-beta.4 / 1.3.13,
1.4.0-beta.5 / 1.3.14
petebacondarwin
force-pushed
the
issue-10869
branch
from
2329e9d to
63d5819
Compare
| @@ -492,6 +495,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { | |||
|
|
|||
| // We will re-render the option elements if the option values or labels change | |||
| scope.$watchCollection(ngOptions.getWatchables, updateOptions); | |||
| scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true); | |||
There was a problem hiding this comment.
The reason we now need this is that we can't assume that the selected model value is not a complex object, in which case the normal ngModel checking would fail to notice the change here.
|
As far as I can tell, this looks good. Your explanation in #10869 (comment) was very helpful. I think it would make sense to add a condensed version of the explanation to the commit message or add your comments as code comments. |
|
Great! Thanks @Narretz - I will add that info to the commit and possibly as comments in code. |
This problem 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, lets name the objects:
```js
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 we begin we have:
```js
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.
```js
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.
This commit fixes this in `ngOptions` by adding a **deep** watch on the
`attr.ngModel` expression...
```js
scope.$watch(attr.ngModel, updateOptions, true);
```
You can see that in this Plunker:
http://plnkr.co/edit/0PE7qN5FXIA23y4RwyN0?p=preview
-------
But this isn't the end of the story. Since `ngModel` and `ngOptions` did
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` points directly to one of the option objects:
```js
// 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:
```js
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' }
]);
```
This commit fixes this in `ngOptions` by making 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...
You can see this in the Plunker here:
http://plnkr.co/edit/YEzEf4dxHTnoW5pbeJDp?p=preview
Closes angular#10869
Closes angular#10893
petebacondarwin
force-pushed
the
issue-10869
branch
from
63d5819 to
6a03ca2
Compare
netman92
pushed a commit
to netman92/angular.js
that referenced
this pull request
Aug 8, 2015
This problem 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, lets name the objects:
```js
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 we begin we have:
```js
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.
```js
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.
This commit fixes this in `ngOptions` by adding a **deep** watch on the
`attr.ngModel` expression...
```js
scope.$watch(attr.ngModel, updateOptions, true);
```
You can see that in this Plunker:
http://plnkr.co/edit/0PE7qN5FXIA23y4RwyN0?p=preview
-------
But this isn't the end of the story. Since `ngModel` and `ngOptions` did
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` points directly to one of the option objects:
```js
// 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:
```js
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' }
]);
```
This commit fixes this in `ngOptions` by making 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...
You can see this in the Plunker here:
http://plnkr.co/edit/YEzEf4dxHTnoW5pbeJDp?p=preview
Closes angular#10869
Closes angular#10893
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #10869