[1.3-beta16] ngModel $parsers regression vs. v1.2

[mgcrea]

I'm having issues with the AngularStrap datepicker in the latest v1.3 beta.

Basically, I've found out that the ngModel $parsers won't be called if $setViewValue is called multiple times with the same value/reference. It was the case in the v1.2 branch.

This is be problematic when we are using objects that change values but keep the same reference (such as Dates).

Check the following plunkers (console.warn):

• v1.2: http://plnkr.co/edit/PuleeGHkLHj3RvYSobs5?p=preview
• v1.3: http://plnkr.co/edit/yRL4JR0aVY3kDGgwLXX2?p=preview

Might be related to this commit

[Narretz]

Yes, it looks like facd904#diff-c244afd8def7f268b16ee91a0341c4b2R1759 introduced the change. It works with 1.3.0-beta.9, and there were no other changes related to forms.

[mgcrea]

1f6a5a1 might provide a workaround with revalidate. Not sure this is good enough though as this is a breaking change.

[shahata]

yes, of course. since we are testing if (ctrl.$$lastCommittedViewValue === value) { return; } we will never commit a new value in case $$lastCommittedViewValue is an object that keeps being reused by later updates.

I can look into this, but I'm not sure that this is the only point in ngModelController that assumes that the viewValue is not a reference to some object that can update independently after $setViewValue is called. I might be wrong, but it means for example, that when this object gets updated independently, all the scope variables that are bound to it get updated as well. Which means that updates cannot be delayed by ngModelOptions. WDYT? Is this really a legitimate way to use this API?

[shahata]

The example provided by @mgcrea for example can be fixed easily by doing controller.$setViewValue(angular.copy(date)) instead of controller.$setViewValue(date)
http://plnkr.co/edit/u44HOYpWrrQQyEFDszV5?p=preview

[mgcrea]

@shahata, yup, did patch AngularStrap with a proper workaround. Thanks!

I do agree that the API usage was a bit far-fetched and I'm OK with the new behavior.

[shahata]

@Narretz WDYT? I believe this is an acceptable breaking change and can be closed...

[Narretz]

It's okay for me, but @matsko or @caitp should have a look at this. I'm not part of the core team. :)

[caitp]

It's nonsense to behave differently for objects and primitive values, but it's pretty terrible to make copies of objects to compare against, too, and we can't rely on better mechanisms in 1.x, and even those other mechanisms would be a bit heavy for this. It basically sucks no matter what.

[petebacondarwin]

I have a proposed solution for this at #8047. Take a look.

[petebacondarwin]

Having discussed this with @shahata, the feeling is that making a copy of the $viewValue within ngModel is too fraught with difficulties. There if you don't make the copy early enough then changes to the object may slip through between recent calls to $setViewValue when you don't want them to. Even if you do make a copy then there are potentially problems with the copy not being an exact replica in the case of the object having dynamic accessor properties, or with property changes on the prototype not being noticed. Finally we would then need to manage the same issue from model to view as well.

So the result is that directive developers (i.e. those writing custom input controls such as the date picker in this issue) are responsible for making a copy of their object before passing it to $setViewValue, which is the workaround suggested by @shahata in this issue and accepted as reasonable by @mgcrea.

Of course, if the $viewValue is not an object, but a string or number then this is a moot point and there is nothing to worry about. Luckily this is case for the vast majority of directives.

It would be good if someone could put together a docs PR that explained this - perhaps in the guide section?