feat(ngModel): bind to getters/setters

[btford]

Currently, you might do this if you want to bind to getters/setters:

controller

// ...
$scope.watch(function () {
  return myModel.getterSetter();
}, function (newValue) {
  $scope.someProp = newValue;
});

$scope.watch('someProp', function (newValue) {
  myModel.getterSetter(newValue);
});
// ...

template

<input ng-model="someProp">

The implementation in this PR changes the semantics of ngModel in the following ways:

If the expression bound to ngModel resolves to a function, the function is invoked to get the current value to be expressed in the DOM. When the binding changes, if the expression bound to ngModel resolves to a function (at that time) the function is invoked with the new value.

This means instead, you could do this:

controller

// ...
$scope.myModel = myModel;
// ...

template

<input ng-model="myModel.getterSetter">

I like that to end developers, this feels like "uniform access" for common cases. I don't like that this means there's a difference in semantics between ngModel and expressions elsewhere in Angular.

This would be a breaking change, but I don't think that this would affect any legitimate use cases. The only case I can think of is if hypothetically, you bind to some property that's originally a function, overwriting it with a string. I can't think of any good reason to write controller code like that.

@IgorMinar suggested a different syntax so that the difference in semantics is obvious. Something like:

<input ng-model="myModel.getterSetter()">

I like that this makes the semantics obvious. I don't like that this still violates the "uniform access principle."

Closes #768

[mary-poppins]

you are doing a good job 👏

[btford]

@IgorMinar @caitp @matsko – feedback pls.

I prefer not to introduce new syntax for the reasons stated above.

I'm worried that btford@fa4b121#diff-c244afd8def7f268b16ee91a0341c4b2R1810 might have performance implications.

[caitp]

the main thing I'm not big on here is the fact that it's specific to forms/ngModel, but maybe it doesn't matter that much. Adding new syntax to the parser would generalize this more, but that's understandably not something we necessarily want to do.

I don't think there should be a major performance hit from the type checks because those strings should be interned and most likely reduced to a pointer comparison, but I could be wrong. Branching might not be super helpful, but it probably won't matter in this case.

I have some questions though, comments forthcoming

[caitp]

I was going to ask what we're doing with getter here, because getter(ctrl.$modelValue) doesn't use the result for anything.

But I realized that this is because this is named getter, and it should really be renamed to reflect how it's being used here --- setter or getterSetter maybe

[btford]

👍 Agreed on the var name

[btford]

the main thing I'm not big on here is the fact that it's specific to forms/ngModel

I think this is reasonable. The main use case I want to address with this approach is where currently you have to write a $watch in your controller to manually call some getter/setter.

[caitp]

I don't have a lot against it, but I think all of these concerns would be lifted if it was implemented as a syntax enhancement in the parser instead. But I don't have any major concerns with shipping it as is

[btford]

What kind of syntax are you thinking?

[caitp]

I don't have anything in particular in mind, but it would have to clearly indicate that the method should be treated as a getter/setter

model=<getterSetter>
model=%getterSetter%
model=gs:getterSetter%
model=#getterSetter

I'm not sure --- it doesn't really matter how it would look, so long as it doesn't conflict with other uses, I guess.

But it's harder to implement this way, so I don't have major reservations about landing it as is.

[btford]

@IgorMinar your input would be appreciated!

[IgorMinar]

just use isFunction here

[IgorMinar]

instead of syntax change, how about we add a new ngModelOption option that would specify that the model is a getter/setter?

that way there is no perf impact, no confusion, everyone is happy it just requires a bit more typing which should however not be that big of a deal especially given that we are trading it off for less confusion/surprises?

[btford]

What about my proposed behavior by default, or you can explicitly provide a ngModelOption specifying whether or not to use the getterSetter behavior?

Then there is no ambiguity and by default much less typing for users who want this behavior.

[IgorMinar]

Since ngModelOptions can be inherited from the form you'd need to specify
it only once per view. That's not bad given how much hassle it saves. No?

On Fri, Jun 27, 2014, 5:22 PM, Brian Ford [email protected] wrote:

What about my proposed behavior by default, or you can explicitly provide
a ngModelOption specifying whether or not to use the getterSetter
behavior?

Then there is no ambiguity and by default much less typing for users who
want this behavior.

—
Reply to this email directly or view it on GitHub
#7991 (comment).

[caitp]

I don't think it's worth worrying about those cases I mentioned until someone actually complains about them, but if you want to add such a solution, it probably doesn't hurt to try it

[btford]

Hmm.

On a semi-related note, there are 3 possible behaviors for any given ngModel:

1. always expect to invoke a getterSetter.
2. always expect to read/assign a property.
3. infer whether to read/assign or invoke based on the type of the bound expression.

See my gross WIP patch above for clarification.

I added a new option, getterSetter, to ngModelOptions that exposes all three behaviors. I expect that the first behavior is rarely desirable, so perhaps it's better to eliminate it entirely.

[btford]

We decided that the first case in my comment above wasn't ever really desirable. We'll keep the current behavior as a default and expose the new behavior via ngModelOptions. As a reminder, ngModelOptions can be scoped to a form so that you don't have to use one on every single ngModel.

[btford]

I've implemented the changes as described, but I'd like to improve the documentation (and commit message) around this before it's merged.

[IgorMinar]

you should also add a test for the default behavior

[IgorMinar]

add the missing default case test + docs and ship it :)

[btford]

okay 🍓

[btford]

Done; I'm planning to merge this by EoD unless anyone has additional feedback.

[Narretz]

I don't really understand what external API means here. Server-Side? Exposed in view?

[btford]

Good call. I like "exposed to the view" better.

[btford]

@Narretz: I addressed your comments. Thanks for the feedback!

[lrlopez]

Shouldn't be this { getterSetter: true }?

Same on next line

[btford]

👍 Good catch!

[btford]

Landed as b9fcf01.

[paglias]

@btford this is a great feature but I would like the option to support different types of getters and setters. Not every getter is avalaible as property() and setter as property(newValue), but for example as property.set(newValue). Of course you can't support every implementation out there but I would like the ability to define my own getters and setters. Something like this:

<input type="text" ng-model-getter="myProperty.get()" ng-model-setter="myProperty.setter">

where if myProperty.setter is a function it should be invoked with the new value while if it is a norma property it just needs the new value to be assigned to it.

What do you think?

[ntrrgc]

This is nice, but here is a subtle bug: if you provide ng-model a function that is the return value of another function call, it fails with Expression is non-assignable, even though ng-model-options has getterSetter set to true.

This is inconvenient when using function factories, e.g. in the following example task.patchMethod() would return a function that can be used either as a getter with no arguments and as a setter with an argument, which would update the specified property (e.g. title) with the value provided in the returned function and then request an HTTP PATCH.

<input type="text" ng-model="task.patchMethod('title')"
 ng-model-options="{ getterSetter: true, updateOn: 'blur' }" />

But given the non-assignable limitation, this does not work.

[caitp]

I feel like trying to support all of these cases (functions returned from other functions returned from other functions and other matryoshka-like behaviour) is not going to be helpful --- I think you could instead use patchMethod when defining your getter/setter to begin with, but there's no point asking ngModel to get the getter/setter by calling it. ngModel already cares about too much stuff already, it would be crazy to give it more things to be concerned with :(

Personal opinion, and maybe Brian or others disagree, but I see this as a kind of feature creep, and would prefer if the limitations were documented and solutions prescribed, rather than adding more hacks to ngModel.

[btford]

I'm not convinced that the case in @ntrrgc's example is a particularly good idea.

[Delagen]

Is it possible in angular 1.2? Missing support of object.defineProperty in IE8 and dropping support of IE8 in angular 1.3 makes ugly without getter setter support

[btford]

Is it possible in angular 1.2?

This feature is not backported to 1.2 by design because it is a breaking change.

If you know what you are doing, you could decorate ngModel in 1.2 to add this feature yourself.