fix(ngOptions): use watchCollection not deep watch of ngModel #11743
Conversation
|
Tests comments, code comments and documentation need to be updated to reflect the change. Otherwise, LGTM |
|
@lgalfaso I added tests and docs. Can you review? |
|
After a superficial look it lgtm, but I wonder if this qualifies as a breaking change. I don't know why I am mentioning it; I probably shouldn't, but I just have to get it out (please ignore 😄): // A few (insignificant) lines of (test) code can be saved and
// readability can be marginally improved (imo), by replacing occurrences of:
scope.$apply(function() {
scope.x = y;
});
// with:
scope.$apply('x = y'); |
|
@gkalpak - yeah I know what you mean about the BC message. I was trying to get away without one. But I guess it doesn't do any harm to highlight this. @gkalpak - yeah I know what you are saying about $apply expressions and I tend to fluctuate back and forth between the two styles. On one hand I want to be consistent and on the other concise. The former is tricky when there are occasions when you cannot express what you want to change on the scope in an Angular expression. |
|
In 1.3.15 it totally doesn't work if you update deeper properties on the model so this is not a breaking change, IMO Compare 1.3.15: http://plnkr.co/edit/zsgnhflQ3M1ClUSrsne0?p=preview |
Using a deep watch caused Angular to enter an infinite recursion in the case that the model contains a circular reference. Using `$watchCollection` instead prevents this from happening. This change means that we will not re-render the directive when there is a change below the first level of properties on the model object. Making such a change is a strange corner case that is unlikely to occur in practice and the directive is not designed to support such a situation. The documentation has been updated to clarify this behaviour. This is not a breaking change since in 1.3.x this scenario did not work at all. Compare 1.3.15: http://plnkr.co/edit/zsgnhflQ3M1ClUSrsne0?p=preview to snapshot: http://plnkr.co/edit/hI48vBc0GscyYTYucJ0U?p=preview Closes angular#11372 Closes angular#11653 Closes angular#11743
petebacondarwin
force-pushed
the
fix11372
branch
from
1f9163e to
e6ab5ab
Compare
|
I was referring to a breaking change since 1.4.0-rc.1 (for people upgrading from that version to 1.4.0-rc.2 or later). But it's a minor one anyway. |
| @@ -901,8 +949,7 @@ describe('ngOptions', function() { | |||
|
|
|||
| // Update the properties on the object in the selected array, rather than replacing the whole object | |||
There was a problem hiding this comment.
I think this comment should be updated, and so the test description.
There was a problem hiding this comment.
@lgalfaso - looking at this a little more I am now thinking that in the specific situation of multiple + track by we should handle the value of the track by changing within each of the items in the collection of selected values.
I will put together an additional PR, which also should eliminate any form of BC to keep @gkalpak happy :-P
|
Just two small comments. Otherwise, LGTM |
Commit 47f9fc3 failed to account for changes to the tracked value of model items in a collection where the select was `multiple`. See angular#11743 (comment)
Commit 47f9fc3 failed to account for changes to the tracked value of model items in a collection where the select was `multiple`. See angular#11743 (comment)
Commit 47f9fc3 failed to account for changes to the tracked value of model items in a collection where the select was `multiple`. See angular#11743 (comment)
Commit 47f9fc3 failed to account for changes to the tracked value of model items in a collection where the select was `multiple`. See #11743 (comment) Closes #11784
Using a deep watch caused Angular to enter an infinite recursion in the case that the model contains a circular reference. Using `$watchCollection` instead prevents this from happening. This change means that we will not re-render the directive when there is a change below the first level of properties on the model object. Making such a change is a strange corner case that is unlikely to occur in practice and the directive is not designed to support such a situation. The documentation has been updated to clarify this behaviour. This is not a breaking change since in 1.3.x this scenario did not work at all. Compare 1.3.15: http://plnkr.co/edit/zsgnhflQ3M1ClUSrsne0?p=preview to snapshot: http://plnkr.co/edit/hI48vBc0GscyYTYucJ0U?p=preview Closes angular#11372 Closes angular#11653 Closes angular#11743
Commit 47f9fc3 failed to account for changes to the tracked value of model items in a collection where the select was `multiple`. See angular#11743 (comment) Closes angular#11784
No description provided.