Skip to content

Commit

Permalink
fix(ngOptions): use watchCollection not deep watch of ngModel
Browse files Browse the repository at this point in the history
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 angular#11372
Closes angular#11653
Closes angular#11743
  • Loading branch information
petebacondarwin committed Apr 29, 2015
1 parent 74eb17d commit 47f9fc3
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 10 deletions.
22 changes: 16 additions & 6 deletions src/ng/directive/ngOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,21 @@ var ngOptionsMinErr = minErr('ngOptions');
* be nested into the `<select>` element. This element will then represent the `null` or "not selected"
* option. See example below for demonstration.
*
* <div class="alert alert-warning">
* **Note:** By default, `ngModel` compares by reference, not value. This is important when binding to an
* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/). When using `track by`
* in an `ngOptions` expression, however, deep equality checks will be performed.
* </div>
* ## Complex Models (objects or collections)
*
* **Note:** By default, `ngModel` watches the model by reference, not value. This is important when
* binding any input directive to a model that is an object or a collection.
*
* Since this is a common situation for `ngOptions` the directive additionally watches the model using
* `$watchCollection` when the select has the `multiple` attribute or when there is a `track by` clause in
* the options expression. This allows ngOptions to trigger a re-rendering of the options even if the actual
* object/collection has not changed identity but only a property on the object or an item in the collection
* changes.
*
* Note that `$watchCollection` does a shallow comparison of the properties of the object (or the items in the collection
* if the model is an array). This means that changing a property deeper inside the object/collection that the
* first level will not trigger a re-rendering.
*
*
* ## `select` **`as`**
*
Expand Down Expand Up @@ -515,7 +525,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
// We also need to watch to see if the internals of the model changes, since
// ngModel only watches for object identity change
if (ngOptions.trackBy) {
scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true);
scope.$watchCollection(attr.ngModel, function() { ngModelCtrl.$render(); });
}
// ------------------------------------------------------------------ //

Expand Down
89 changes: 85 additions & 4 deletions test/ng/directive/ngOptionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,54 @@ describe('ngOptions', function() {
});


it('should re-render if an item in an array source is added/removed', function() {
createSelect({
'ng-model': 'selected',
'multiple': true,
'ng-options': 'item.id as item.label for item in arr'
});

scope.$apply(function() {
scope.selected = [10];
});
expect(element).toEqualSelectValue([10], true);

scope.$apply(function() {
scope.selected.push(20);
});
expect(element).toEqualSelectValue([10, 20], true);


scope.$apply(function() {
scope.selected.shift();
});
expect(element).toEqualSelectValue([20], true);
});


it('should handle a options containing circular references', function() {
scope.arr[0].ref = scope.arr[0];
scope.selected = [scope.arr[0]];
createSelect({
'ng-model': 'selected',
'multiple': true,
'ng-options': 'item as item.label for item in arr'
});
expect(element).toEqualSelectValue([scope.arr[0]], true);

scope.$apply(function() {
scope.selected.push(scope.arr[1]);
});
expect(element).toEqualSelectValue([scope.arr[0], scope.arr[1]], true);


scope.$apply(function() {
scope.selected.pop();
});
expect(element).toEqualSelectValue([scope.arr[0]], true);
});


it('should support single select with object source', function() {
createSelect({
'ng-model': 'selected',
Expand Down Expand Up @@ -901,8 +949,7 @@ describe('ngOptions', function() {

// Update the properties on the object in the selected array, rather than replacing the whole object
scope.$apply(function() {
scope.selected[0].id = 20;
scope.selected[0].label = 'new twenty';
scope.selected[0] = {id: 20, label: 'new twenty'};
});

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

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

createSelect({
'ng-model': 'selected',
Expand All @@ -1064,7 +1111,7 @@ describe('ngOptions', function() {

});

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

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


it('should handle options containing circular references (single)', function() {
scope.arr[0].ref = scope.arr[0];
createSelect({
'ng-model': 'selected',
'ng-options': 'item for item in arr track by item.id'
});

expect(function() {
scope.$apply(function() {
scope.selected = scope.arr[0];
});
}).not.toThrow();
});


it('should handle options containing circular references (multiple)', function() {
scope.arr[0].ref = scope.arr[0];
createSelect({
'ng-model': 'selected',
'multiple': true,
'ng-options': 'item for item in arr track by item.id'
});

expect(function() {
scope.$apply(function() {
scope.selected = [scope.arr[0]];
});

scope.$apply(function() {
scope.selected.push(scope.arr[1]);
});
}).not.toThrow();
});
});


Expand Down

0 comments on commit 47f9fc3

Please sign in to comment.