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

fix(ngOptions): ensure that tracked properties are always watched #11784

Closed
wants to merge 1 commit into from
Closed

fix(ngOptions): ensure that tracked properties are always watched #11784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
106 changes: 66 additions & 40 deletions src/ng/directive/ngOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,13 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
// Get the value by which we are going to track the option
// if we have a trackFn then use that (passing scope and locals)
// otherwise just hash the given viewValue
var getTrackByValue = trackBy ?
function(viewValue, locals) { return trackByFn(scope, locals); } :
function getHashOfValue(viewValue) { return hashKey(viewValue); };
var getTrackByValueFn = trackBy ?
function(value, locals) { return trackByFn(scope, locals); } :
function getHashOfValue(value) { return hashKey(value); };
var getTrackByValue = function(value, key) {
return getTrackByValueFn(value, getLocals(value, key));
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getTrackByValueFn and getTrackByValue are both useful functions.
The first allows us to get performance benefits by not recomputing the locals unnecessarily
The second gives us a clean function that we can expose outside this function without having to expose getLocals

var displayFn = $parse(match[2] || match[1]);
var groupByFn = $parse(match[3] || '');
var disableWhenFn = $parse(match[4] || '');
Expand All @@ -290,6 +294,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {

return {
trackBy: trackBy,
getTrackByValue: getTrackByValue,
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 All @@ -299,7 +304,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {

Object.keys(values).forEach(function getWatchable(key) {
var locals = getLocals(values[key], key);
var selectValue = getTrackByValue(values[key], locals);
var selectValue = getTrackByValueFn(values[key], locals);
watchedArray.push(selectValue);

// Only need to watch the displayFn if there is a specific label expression
Expand Down Expand Up @@ -347,7 +352,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
var value = optionValues[key];
var locals = getLocals(value, key);
var viewValue = viewValueFn(scope, locals);
var selectValue = getTrackByValue(viewValue, locals);
var selectValue = getTrackByValueFn(viewValue, locals);
var label = displayFn(scope, locals);
var group = groupByFn(scope, locals);
var disabled = disableWhenFn(scope, locals);
Expand All @@ -361,7 +366,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
items: optionItems,
selectValueMap: selectValueMap,
getOptionFromViewValue: function(value) {
return selectValueMap[getTrackByValue(value, getLocals(value))];
return selectValueMap[getTrackByValue(value)];
},
getViewValueFromOption: function(option) {
// If the viewValue could be an object that may be mutated by the application,
Expand Down Expand Up @@ -439,44 +444,54 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
};


selectCtrl.writeValue = function writeNgOptionsValue(value) {
var option = options.getOptionFromViewValue(value);
// Update the controller methods for multiple selectable options
if (!multiple) {

if (option && !option.disabled) {
if (selectElement[0].value !== option.selectValue) {
removeUnknownOption();
removeEmptyOption();
selectCtrl.writeValue = function writeNgOptionsValue(value) {
var option = options.getOptionFromViewValue(value);

selectElement[0].value = option.selectValue;
option.element.selected = true;
option.element.setAttribute('selected', 'selected');
}
} else {
if (value === null || providedEmptyOption) {
removeUnknownOption();
renderEmptyOption();
if (option && !option.disabled) {
if (selectElement[0].value !== option.selectValue) {
removeUnknownOption();
removeEmptyOption();

selectElement[0].value = option.selectValue;
option.element.selected = true;
option.element.setAttribute('selected', 'selected');
}
} else {
removeEmptyOption();
renderUnknownOption();
if (value === null || providedEmptyOption) {
removeUnknownOption();
renderEmptyOption();
} else {
removeEmptyOption();
renderUnknownOption();
}
}
}
};
};

selectCtrl.readValue = function readNgOptionsValue() {
selectCtrl.readValue = function readNgOptionsValue() {

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

if (selectedOption && !selectedOption.disabled) {
removeEmptyOption();
removeUnknownOption();
return options.getViewValueFromOption(selectedOption);
}
return null;
};
if (selectedOption && !selectedOption.disabled) {
removeEmptyOption();
removeUnknownOption();
return options.getViewValueFromOption(selectedOption);
}
return null;
};

// If we are using `track by` then we must watch the tracked value on the model
// since ngModel only watches for object identity change
if (ngOptions.trackBy) {
scope.$watch(
function() { return ngOptions.getTrackByValue(ngModelCtrl.$viewValue); },
function() { ngModelCtrl.$render(); }
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So now, when the select is not multiple, there is only a simple watch on the tracked value.

}

// Update the controller methods for multiple selectable options
if (multiple) {
} else {

ngModelCtrl.$isEmpty = function(value) {
return !value || value.length === 0;
Expand Down Expand Up @@ -508,6 +523,22 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {

return selections;
};

// If we are using `track by` then we must watch these tracked values on the model
// since ngModel only watches for object identity change
if (ngOptions.trackBy) {

scope.$watchCollection(function() {
if (isArray(ngModelCtrl.$viewValue)) {
return ngModelCtrl.$viewValue.map(function(value) {
return ngOptions.getTrackByValue(value);
});
}
}, function() {
ngModelCtrl.$render();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, when the select is multiple, there is a $watchCollection on the array of tracked values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I don't get it, but it looks like if the $viewValue is not an array, nothing is actually watched, right? So is this only valid for select[multiple]? And if so, could we add this $watch only if it is multiple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only added if multiple. I reorganized the if statement above. This is in the else clause now.


}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using $watchCollection on the model was misguided.
This would:

  • unnecessarily watch all the properties on a non-multiple select's model
  • not react to changes if the tracked value was based on a deeper property in the model on a non-multiple select
  • not react to changes in a multple select if only the tracked property value was changed on an item in the model collection.

}


Expand All @@ -534,11 +565,6 @@ 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);

// 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.$watchCollection(attr.ngModel, function() { ngModelCtrl.$render(); });
}
// ------------------------------------------------------------------ //


Expand Down
10 changes: 5 additions & 5 deletions test/ng/directive/ngOptionsSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,9 +988,9 @@ describe('ngOptions', function() {

expect(element.val()).toEqual(['10']);

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

Choose a reason for hiding this comment

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

The earlier commit that changed this line was wrong.

});

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

it('should re-render if a propery of the model is changed when using trackBy', function() {
it('should re-render if the tracked property of the model is changed when using trackBy', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not actually interested in watching any old property on the model. The important thing is the track by expression.


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

scope.$apply(function() {
scope.selected = scope.arr[0];
scope.selected = {id: 10, label: 'ten'};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to use a totally different object here (which is matched by the id property to the option) so that we can then change the value of the id property of the model without changing the id property on the option.

});

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

scope.$apply(function() {
scope.selected.label = 'changed';
scope.arr[0].id = 20;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are using track by here, the model and the option are different objects.

So we are not interested in the label of the selected item, as this doesn't affect the visual display of the options, which are only interested in the label property on the options themselves.

});

// update render due to equality watch
Expand Down