ngModelOption's getterSetter does not work with Factory objects #9394
Comments
lightswitch05
changed the title
|
This is kind of a dupe of #8552 (sort of). So, the parser isn't the one invoking the function, and we have no way to get the context of the function :( It would be good to make this work nicely everywhere though |
|
I was working with @lightswitch05 on this, and if it's not currently possible to get the context, maybe the docs should be updated to reflect that. I volunteer @lightswitch05 to update the docs 😄 |
|
@caitp I propose this as a solution:
thoughts? |
|
I don't think that's particularly useful --- I would rather just change the parser so that it can return the context so that we can get it when we need it. |
|
so I don't know how big of a problem this is, but I guess it couldn't hurt to fix it --- I just don't think it's a huge priority. Maybe I'll write a patch for this on the weekend and see what people think |
|
I'm currently working around this by storing a reference to |
|
Something like this could work, although I'm not sure how many other things it would break... In var code = '';
forEach(pathKeys, function(key, index) {
ensureSafeMemberName(key, fullExp);
code += 'var p = s;\n';
code += 'if(s == null) return undefined;\n';
if (index == 0) {
// but if we are first then we check locals first, and if so read it first
code += 's=((l&&l.hasOwnProperty("' + key + '"))?l:s)' + '.' + key + ';\n';
} else {
// we simply dereference 's' on any .dot notation
code += 'p = s;\n'
code += 's=s.' + key + ';\n'
}
});
code += "if (typeof s === 'function' && p != null) return s.bind(p);\n"
code += 'return s;'; |
|
would be a performance killer, not the right way to do this |
|
I mean, it is technically the best way to solve the problem, but also would kill performance :( |
|
Yeah 😄 Maybe you could reparse the ngModel expression at call time with a special argument to the lexer... |
|
in my view, it would be easy to provide a separate "getter" function (lets call it Obviously this sucks because getting the context (and calling a function using that context) would be the responsibility of the developer, but I don't really see a better way of doing this that doesn't suck the life out of applications. Alternatively we could just call this behaviour unsupported and never handle it, but I don't think that would make people happy :( WDYT @btford |
|
You could do something hacky in var parsedNgModel = $parse($attr.ngModel),
pendingDebounce = null,
ctrl = this,
parsedNgModelParent = null;
if (ctrl.$options.getterSetter) {
parsedNgModelParent = parseParent($attr.ngModel);
}
var ngModelGet = function ngModelGet() {
var modelValue = parsedNgModel($scope);
if (ctrl.$options && ctrl.$options.getterSetter && isFunction(modelValue)) {
modelValue = modelValue.call(parsedNgModelParent);
}
return modelValue;
};That way you can still have the nice benefit of the lexer providing the context (not the developer), but you limit the performance impact to a second, optional call to Still pretty gross 😒 |
|
I think this reasonably should be supported. |
|
I have a scenario when this really is a blocker - when you use TypeScript. I use TypeScript to implement all of my application logic including controllers, models etc. TypeScript generates methods using prototype. None of getters/setters then work with AngularJS due to this issue. When you mention that it would be a performance killer, have you actually tried how much? I made a small test: http://plnkr.co/ABCvAb6FrYO2PNB7J7ic Approach with bind() is slower as we expect. In IE11 it's about two times slower than current approach. In Chrome it's about 10 times slower. The test just runs each version one million times in a loop so it's kind of artificial but shows the difference. For that 1 million calls it makes about 1 second in Chrome. Now the question is, if you run full AngularJS processing for single getter, what is the relative slowdown? If whole processing executed 1000 times today takes let's say 1 second and with this change would take 1.1 second, I would go for it. Today you generate code for the getter as a string and let runtime parse it and execute it. That sounds like a much bigger performance hit than calling bind() (though I haven't tested that). Current behavior that prevents you to use object methods correctly makes getter/setter option kind of useless. |
|
Hmm. Something like this would be more explicit, and let you specify the caller for your getter function if you wanted... var parsedNgModel = $parse($attr.ngModel),
parsedNgModelContext = $parse($attr.ngModelContext),
pendingDebounce = null,
ctrl = this;
var ngModelGet = function ngModelGet() {
var modelValue = parsedNgModel($scope);
if (ctrl.$options && ctrl.$options.getterSetter && isFunction(modelValue)) {
var context = parsedNgModelContext($scope) || this;
modelValue = modelValue.call(context);
}
return modelValue;
};
var ngModelSet = function ngModelSet(newValue) {
var getterSetter;
if (ctrl.$options && ctrl.$options.getterSetter &&
isFunction(getterSetter = parsedNgModel($scope))) {
var context = parsedNgModelContext($scope) || this;
getterSetter.call(context, ctrl.$modelValue);
} else {
parsedNgModel.assign($scope, ctrl.$modelValue);
}
};That would make something like this work as you were hoping, where <input ng-model="someFactory.getterSetter"
ng-model-options="{ getterSetter: true }"
ng-model-context="someFactory">I think that's what @caitp was originally proposing, and this doesn't require touching |
Adds an optional attribute, `ng-model-context`, that will be evaluated to provide the calling context used when invoking the ngModel getter/setter function. When not provided, falls back to the existing behavior of invoking getter/setter functions from the global context. Closes angular#9394
When using a getter/setter function for ngModel, attempt to determine the appropriate context by
parsing the ngModel expression. If the context cannot be found, fallback to using the current scope.
For example, '<input ng-model="someObject.value" ng-model-options="{ getterSetter: true }">' will
use 'someObject' as the calling context.
Non-assignable ngModel expressions will always fallback to using the current scope as the context.
For example, '<input ng-model="someObject.getValueFn()" ng-model-options="{ getterSetter: true }">'
will invoke the result of 'someObject.getValueFn()' from the current scope.
Closes angular#9394
BREAKING CHANGE: previously, getter/setter functions would always be called from the global context.
This behaviour was unexpected by some users, as described in angular#9394, and is not particularly nice
anyways. Applications that relied on this behaviour can use `$window` instead of `this` to access
the global object... but they probably shouldn't be storing global state anyways!
When using a getter/setter function for ngModel, attempt to determine the appropriate context by
parsing the ngModel expression. If the context cannot be found, fallback to using the current scope.
For example, '<input ng-model="someObject.value" ng-model-options="{ getterSetter: true }">' will
use 'someObject' as the calling context.
Non-assignable ngModel expressions will always fallback to using the current scope as the context.
For example, '<input ng-model="someObject.getValueFn()" ng-model-options="{ getterSetter: true }">'
will invoke the result of 'someObject.getValueFn()' from the current scope.
Closes angular#9394
BREAKING CHANGE: previously, getter/setter functions would always be called from the global context.
This behaviour was unexpected by some users, as described in angular#9394, and is not particularly nice
anyways. Applications that relied on this behaviour can use `$window` instead of `this` to access
the global object... but they probably shouldn't be storing global state anyways!
…getter/setter bindings
Along with getterSetter, allow users to provide an expression via the getterSetterContext option.
This expression is evaluated to determine the context that should be used when invoking the ngModel
as a getter/setter function.
For example, <input ng-model="someObject.value" ng-model-options="{ getterSetter: true }"> would
previously invoke 'someObject.value()' from the global context. Now, users can specify context, like
ng-model-options="{ getterSetter: true, getterSetterContext: 'someObject'}", which would invoke
'someObject.value()' using 'someObject' as the calling context.
If getterSetterContext is not provided, fallback to using the current scope as the context.
Closes angular#9394
BREAKING CHANGE: previously, getter/setter functions would always be called from the global context.
This behaviour was unexpected by some users, as described in angular#9394, and is not particularly nice
anyways. Applications that relied on this behaviour can use `$window` instead of `this` to access
the global object... but they probably shouldn't be storing global state anyways!
…getter/setter bindings
Along with getterSetter, allow users to provide an expression via the getterSetterContext option.
This expression is evaluated to determine the context that should be used when invoking the ngModel
as a getter/setter function.
For example, <input ng-model="someObject.value" ng-model-options="{ getterSetter: true }"> would
previously invoke 'someObject.value()' from the global context. Now, users can specify context, like
ng-model-options="{ getterSetter: true, getterSetterContext: 'someObject'}", which would invoke
'someObject.value()' using 'someObject' as the calling context.
If getterSetterContext is not provided, fallback to using the current scope as the context.
Closes angular#9394
BREAKING CHANGE: previously, getter/setter functions would always be called from the global context.
This behaviour was unexpected by some users, as described in angular#9394, and is not particularly nice
anyways. Applications that relied on this behaviour can use `$window` instead of `this` to access
the global object... but they probably shouldn't be storing global state anyways!
Closes angular#9394 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`.
Closes angular#9394 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`.
Closes angular#9394 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`.
Closes angular#9394 BREAKING CHANGE: - the ngModelController getterSetter property is read once at link time and can not change afterwards - previously ngModel invoked the getterSetter in the global context - the ngModel getterSetter expression must work when `()` (a method invokation) is appended to it
Closes angular#9394 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`.
Closes angular#9394 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`.
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`.
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`.
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`.
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`.
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`.
Overview of the Issue
When using
ng-model-options="{ getterSetter: true }", the getter & setter methods do not run within the correct scope if defined in a factory. This causesthisto beundefinedwithin the getter & setter method - preventing anything from being set or retrieved. (TypeError: Cannot read property 'x' of undefined)Motivation for or Use Case
While some getter & setter methods should be defined within the controller & scope - more complex objects should contain their own getter and setter methods defined within their factories for a better separation of concerns.
Angular Version
I've tried this with
1.3.0-rc.4&1.3.0-rc.3Browsers and Operating System
This appears to be Angular.js specific and not a browser problem. However, I am using chrome
37.0.2062.124 mon windows 7 64bit.Reproduce the Error
Here is a plunker that has getter & setter methods defined within a object from a factory that does not work: http://plnkr.co/edit/RmoegMYEgF9s7s50KQNj?p=preview
Here is another version that has the object defined within the controller (no factory) that works fine: http://plnkr.co/edit/4f8i384nsRwCoaQtPoxx?p=preview
The text was updated successfully, but these errors were encountered: