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

perf(ngOptions): only perform deep equality check on ngModel if using track by #11448

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/ng/directive/ngOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ var ngOptionsMinErr = minErr('ngOptions');
* option. See example below for demonstration.
*
* <div class="alert alert-warning">
* **Note:** `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/).
* **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>
*
* ## `select` **`as`**
Expand Down Expand Up @@ -275,6 +276,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
}

return {
trackBy: trackBy,
getWatchables: $parse(valuesFn, function(values) {
// Create a collection of things that we would like to watch (watchedArray)
// so that they can all be watched using a single $watchCollection
Expand Down Expand Up @@ -500,8 +502,9 @@ 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
scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true);

if (ngOptions.trackBy) {
scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true);
}
// ------------------------------------------------------------------ //


Expand Down
5 changes: 3 additions & 2 deletions src/ng/directive/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,9 @@ var SelectController =
* option. See example below for demonstration.
*
* <div class="alert alert-warning">
* **Note:** `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/).
* **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>
*
*/
Expand Down
48 changes: 46 additions & 2 deletions test/ng/directive/ngOptionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,50 @@ describe('ngOptions', function() {
});
}).not.toThrow();
});

it('should setup equality watches on ngModel changes if using trackBy', function() {

createSelect({
'ng-model': 'selected',
'ng-options': 'item for item in arr track by item.id'
});

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

spyOn(element.controller('ngModel'), '$render');

scope.$apply(function() {
scope.selected.label = 'changed';
});

// update render due to equality watch
expect(element.controller('ngModel').$render).toHaveBeenCalled();

});

it('should not setup equality watches on ngModel changes if not using trackBy', function() {

createSelect({
'ng-model': 'selected',
'ng-options': 'item for item in arr'
});

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

spyOn(element.controller('ngModel'), '$render');

scope.$apply(function() {
scope.selected.label = 'changed';
});

// no render update as no equality watch
expect(element.controller('ngModel').$render).not.toHaveBeenCalled();
});

});


Expand Down Expand Up @@ -1657,7 +1701,7 @@ describe('ngOptions', function() {
scope.values.pop();
});

expect(element.val()).toEqual('');
expect(element.val()).toEqualUnknownValue();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wish I had a better understanding of unknownValue usage in select + options. Had to revert the spec back to its form in ef894c8.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so the reason for this is that, if the model is null then element.val() is empty but if the model contains a value that doesn't match any option then element.val() is '?'.

When we remove the option the value becomes invalid so we get '?' but then the model is cleared out and becomes null. If we have a watch on the model then this will cause the ngModel to run $render, which update the element value to ''.

I think that what we need is to trigger a new call to $render when the selected option is removed...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually what we need to do is fix the bit of code inside updateOptions (https://github.com/angular/angular.js/pull/11448/files#diff-0a9e86a58b2663074f51454f7a18a0efR646) that checks if the model has changed

expect(scope.selected).toEqual(null);

// Check after model change
Expand All @@ -1671,7 +1715,7 @@ describe('ngOptions', function() {
scope.values.pop();
});

expect(element.val()).toEqual('');
expect(element.val()).toEqualUnknownValue();
expect(scope.selected).toEqual(null);
});

Expand Down