Skip to content

Commit f91637b

Browse files
committed
fix(ngModelOptions): preserve context of getter/setters
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this! Closes angular#9394 Closes angular#9865 BREAKING CHANGE: previously, ngModel invoked getter/setters in the global context. For example: ```js <input ng-model="model.value" ng-model-options="{ getterSetter: true }"> ``` would previously invoke `model.value()` in the global context. Now, ngModel invokes `value` with `model` as the context. It's unlikely that real apps relied on this behavior. If they did they can use `.bind` to explicilty bind a getter/getter to the global context, or just reference globals normally without `this`.
1 parent e3764e3 commit f91637b

File tree

2 files changed

+61
-25
lines changed

2 files changed

+61
-25
lines changed

src/ng/directive/input.js

+25-24
Original file line numberDiff line numberDiff line change
@@ -1740,32 +1740,33 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
17401740

17411741

17421742
var parsedNgModel = $parse($attr.ngModel),
1743+
parsedNgModelAssign = parsedNgModel.assign,
1744+
ngModelGet = parsedNgModel,
1745+
ngModelSet = parsedNgModelAssign,
17431746
pendingDebounce = null,
17441747
ctrl = this;
17451748

1746-
var ngModelGet = function ngModelGet() {
1747-
var modelValue = parsedNgModel($scope);
1748-
if (ctrl.$options && ctrl.$options.getterSetter && isFunction(modelValue)) {
1749-
modelValue = modelValue();
1750-
}
1751-
return modelValue;
1752-
};
1753-
1754-
var ngModelSet = function ngModelSet(newValue) {
1755-
var getterSetter;
1756-
if (ctrl.$options && ctrl.$options.getterSetter &&
1757-
isFunction(getterSetter = parsedNgModel($scope))) {
1758-
1759-
getterSetter(ctrl.$modelValue);
1760-
} else {
1761-
parsedNgModel.assign($scope, ctrl.$modelValue);
1762-
}
1763-
};
1764-
17651749
this.$$setOptions = function(options) {
17661750
ctrl.$options = options;
1767-
1768-
if (!parsedNgModel.assign && (!options || !options.getterSetter)) {
1751+
if (options && options.getterSetter) {
1752+
var invokeModelGetter = $parse($attr.ngModel + '()'),
1753+
invokeModelSetter = $parse($attr.ngModel + '($$$p)');
1754+
1755+
ngModelGet = function($scope) {
1756+
var modelValue = parsedNgModel($scope);
1757+
if (isFunction(modelValue)) {
1758+
modelValue = invokeModelGetter($scope);
1759+
}
1760+
return modelValue;
1761+
};
1762+
ngModelSet = function($scope, newValue) {
1763+
if (isFunction(parsedNgModel($scope))) {
1764+
invokeModelSetter($scope, {$$$p: ctrl.$modelValue});
1765+
} else {
1766+
parsedNgModelAssign($scope, ctrl.$modelValue);
1767+
}
1768+
};
1769+
} else if (!parsedNgModel.assign) {
17691770
throw $ngModelMinErr('nonassign', "Expression '{0}' is non-assignable. Element: {1}",
17701771
$attr.ngModel, startingTag($element));
17711772
}
@@ -2164,7 +2165,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
21642165
}
21652166
if (isNumber(ctrl.$modelValue) && isNaN(ctrl.$modelValue)) {
21662167
// ctrl.$modelValue has not been touched yet...
2167-
ctrl.$modelValue = ngModelGet();
2168+
ctrl.$modelValue = ngModelGet($scope);
21682169
}
21692170
var prevModelValue = ctrl.$modelValue;
21702171
var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid;
@@ -2192,7 +2193,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
21922193
};
21932194

21942195
this.$$writeModelToScope = function() {
2195-
ngModelSet(ctrl.$modelValue);
2196+
ngModelSet($scope, ctrl.$modelValue);
21962197
forEach(ctrl.$viewChangeListeners, function(listener) {
21972198
try {
21982199
listener();
@@ -2288,7 +2289,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
22882289
// ng-change executes in apply phase
22892290
// 4. view should be changed back to 'a'
22902291
$scope.$watch(function ngModelWatch() {
2291-
var modelValue = ngModelGet();
2292+
var modelValue = ngModelGet($scope);
22922293

22932294
// if scope model value and ngModel value are out of sync
22942295
// TODO(perf): why not move this to the action fn?

test/ng/directive/inputSpec.js

+36-1
Original file line numberDiff line numberDiff line change
@@ -2059,7 +2059,7 @@ describe('input', function() {
20592059
expect(inputElm.val()).toBe('a');
20602060
});
20612061

2062-
it('should always try to invoke a model if getterSetter is true', function() {
2062+
it('should try to invoke a function model if getterSetter is true', function() {
20632063
compileInput(
20642064
'<input type="text" ng-model="name" ' +
20652065
'ng-model-options="{ getterSetter: true }" />');
@@ -2074,6 +2074,12 @@ describe('input', function() {
20742074
expect(inputElm.val()).toBe('b');
20752075
expect(spy).toHaveBeenCalledWith('a');
20762076
expect(scope.name).toBe(spy);
2077+
});
2078+
2079+
it('should assign to non-function models if getterSetter is true', function() {
2080+
compileInput(
2081+
'<input type="text" ng-model="name" ' +
2082+
'ng-model-options="{ getterSetter: true }" />');
20772083

20782084
scope.name = 'c';
20792085
changeInputValueTo('d');
@@ -2093,6 +2099,35 @@ describe('input', function() {
20932099
'ng-model-options="{ getterSetter: true }" />');
20942100
});
20952101

2102+
it('should invoke a model in the correct context if getterSetter is true', function() {
2103+
compileInput(
2104+
'<input type="text" ng-model="someService.getterSetter" ' +
2105+
'ng-model-options="{ getterSetter: true }" />');
2106+
2107+
scope.someService = {
2108+
value: 'a',
2109+
getterSetter: function(newValue) {
2110+
this.value = newValue || this.value;
2111+
return this.value;
2112+
}
2113+
};
2114+
spyOn(scope.someService, 'getterSetter').andCallThrough();
2115+
scope.$apply();
2116+
2117+
expect(inputElm.val()).toBe('a');
2118+
expect(scope.someService.getterSetter).toHaveBeenCalledWith();
2119+
expect(scope.someService.value).toBe('a');
2120+
2121+
changeInputValueTo('b');
2122+
expect(scope.someService.getterSetter).toHaveBeenCalledWith('b');
2123+
expect(scope.someService.value).toBe('b');
2124+
2125+
scope.someService.value = 'c';
2126+
scope.$apply();
2127+
expect(inputElm.val()).toBe('c');
2128+
expect(scope.someService.getterSetter).toHaveBeenCalledWith();
2129+
});
2130+
20962131
it('should assign invalid values to the scope if allowInvalid is true', function() {
20972132
compileInput('<input type="text" name="input" ng-model="value" maxlength="1" ' +
20982133
'ng-model-options="{allowInvalid: true}" />');

0 commit comments

Comments
 (0)