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

Commit 47f9fc3

Browse files
fix(ngOptions): use watchCollection not deep watch of ngModel
Using a deep watch caused Angular to enter an infinite recursion in the case that the model contains a circular reference. Using `$watchCollection` instead prevents this from happening. This change means that we will not re-render the directive when there is a change below the first level of properties on the model object. Making such a change is a strange corner case that is unlikely to occur in practice and the directive is not designed to support such a situation. The documentation has been updated to clarify this behaviour. This is not a breaking change since in 1.3.x this scenario did not work at all. Compare 1.3.15: http://plnkr.co/edit/zsgnhflQ3M1ClUSrsne0?p=preview to snapshot: http://plnkr.co/edit/hI48vBc0GscyYTYucJ0U?p=preview Closes #11372 Closes #11653 Closes #11743
1 parent 74eb17d commit 47f9fc3

File tree

2 files changed

+101
-10
lines changed

2 files changed

+101
-10
lines changed

src/ng/directive/ngOptions.js

+16-6
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,21 @@ var ngOptionsMinErr = minErr('ngOptions');
3131
* be nested into the `<select>` element. This element will then represent the `null` or "not selected"
3232
* option. See example below for demonstration.
3333
*
34-
* <div class="alert alert-warning">
35-
* **Note:** By default, `ngModel` compares by reference, not value. This is important when binding to an
36-
* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/). When using `track by`
37-
* in an `ngOptions` expression, however, deep equality checks will be performed.
38-
* </div>
34+
* ## Complex Models (objects or collections)
35+
*
36+
* **Note:** By default, `ngModel` watches the model by reference, not value. This is important when
37+
* binding any input directive to a model that is an object or a collection.
38+
*
39+
* Since this is a common situation for `ngOptions` the directive additionally watches the model using
40+
* `$watchCollection` when the select has the `multiple` attribute or when there is a `track by` clause in
41+
* the options expression. This allows ngOptions to trigger a re-rendering of the options even if the actual
42+
* object/collection has not changed identity but only a property on the object or an item in the collection
43+
* changes.
44+
*
45+
* Note that `$watchCollection` does a shallow comparison of the properties of the object (or the items in the collection
46+
* if the model is an array). This means that changing a property deeper inside the object/collection that the
47+
* first level will not trigger a re-rendering.
48+
*
3949
*
4050
* ## `select` **`as`**
4151
*
@@ -515,7 +525,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
515525
// We also need to watch to see if the internals of the model changes, since
516526
// ngModel only watches for object identity change
517527
if (ngOptions.trackBy) {
518-
scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true);
528+
scope.$watchCollection(attr.ngModel, function() { ngModelCtrl.$render(); });
519529
}
520530
// ------------------------------------------------------------------ //
521531

test/ng/directive/ngOptionsSpec.js

+85-4
Original file line numberDiff line numberDiff line change
@@ -776,6 +776,54 @@ describe('ngOptions', function() {
776776
});
777777

778778

779+
it('should re-render if an item in an array source is added/removed', function() {
780+
createSelect({
781+
'ng-model': 'selected',
782+
'multiple': true,
783+
'ng-options': 'item.id as item.label for item in arr'
784+
});
785+
786+
scope.$apply(function() {
787+
scope.selected = [10];
788+
});
789+
expect(element).toEqualSelectValue([10], true);
790+
791+
scope.$apply(function() {
792+
scope.selected.push(20);
793+
});
794+
expect(element).toEqualSelectValue([10, 20], true);
795+
796+
797+
scope.$apply(function() {
798+
scope.selected.shift();
799+
});
800+
expect(element).toEqualSelectValue([20], true);
801+
});
802+
803+
804+
it('should handle a options containing circular references', function() {
805+
scope.arr[0].ref = scope.arr[0];
806+
scope.selected = [scope.arr[0]];
807+
createSelect({
808+
'ng-model': 'selected',
809+
'multiple': true,
810+
'ng-options': 'item as item.label for item in arr'
811+
});
812+
expect(element).toEqualSelectValue([scope.arr[0]], true);
813+
814+
scope.$apply(function() {
815+
scope.selected.push(scope.arr[1]);
816+
});
817+
expect(element).toEqualSelectValue([scope.arr[0], scope.arr[1]], true);
818+
819+
820+
scope.$apply(function() {
821+
scope.selected.pop();
822+
});
823+
expect(element).toEqualSelectValue([scope.arr[0]], true);
824+
});
825+
826+
779827
it('should support single select with object source', function() {
780828
createSelect({
781829
'ng-model': 'selected',
@@ -901,8 +949,7 @@ describe('ngOptions', function() {
901949

902950
// Update the properties on the object in the selected array, rather than replacing the whole object
903951
scope.$apply(function() {
904-
scope.selected[0].id = 20;
905-
scope.selected[0].label = 'new twenty';
952+
scope.selected[0] = {id: 20, label: 'new twenty'};
906953
});
907954

908955
// The value of the select should change since the id property changed
@@ -1042,7 +1089,7 @@ describe('ngOptions', function() {
10421089
}).not.toThrow();
10431090
});
10441091

1045-
it('should setup equality watches on ngModel changes if using trackBy', function() {
1092+
it('should re-render if a propery of the model is changed when using trackBy', function() {
10461093

10471094
createSelect({
10481095
'ng-model': 'selected',
@@ -1064,7 +1111,7 @@ describe('ngOptions', function() {
10641111

10651112
});
10661113

1067-
it('should not setup equality watches on ngModel changes if not using trackBy', function() {
1114+
it('should not re-render if a property of the model is changed when not using trackBy', function() {
10681115

10691116
createSelect({
10701117
'ng-model': 'selected',
@@ -1085,6 +1132,40 @@ describe('ngOptions', function() {
10851132
expect(element.controller('ngModel').$render).not.toHaveBeenCalled();
10861133
});
10871134

1135+
1136+
it('should handle options containing circular references (single)', function() {
1137+
scope.arr[0].ref = scope.arr[0];
1138+
createSelect({
1139+
'ng-model': 'selected',
1140+
'ng-options': 'item for item in arr track by item.id'
1141+
});
1142+
1143+
expect(function() {
1144+
scope.$apply(function() {
1145+
scope.selected = scope.arr[0];
1146+
});
1147+
}).not.toThrow();
1148+
});
1149+
1150+
1151+
it('should handle options containing circular references (multiple)', function() {
1152+
scope.arr[0].ref = scope.arr[0];
1153+
createSelect({
1154+
'ng-model': 'selected',
1155+
'multiple': true,
1156+
'ng-options': 'item for item in arr track by item.id'
1157+
});
1158+
1159+
expect(function() {
1160+
scope.$apply(function() {
1161+
scope.selected = [scope.arr[0]];
1162+
});
1163+
1164+
scope.$apply(function() {
1165+
scope.selected.push(scope.arr[1]);
1166+
});
1167+
}).not.toThrow();
1168+
});
10881169
});
10891170

10901171

0 commit comments

Comments
 (0)