Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Error using ngOptions with objects containing circular references #11372

Closed
dubejf opened this issue Mar 19, 2015 · 12 comments
Closed

Error using ngOptions with objects containing circular references #11372

dubejf opened this issue Mar 19, 2015 · 12 comments
Assignees
@lgalfaso
Labels
component: forms severity: regression type: bug
Milestone
1.4.0-rc.2

Comments

@dubejf
Copy link

dubejf commented Mar 19, 2015

The new ngOptions in Angular 1.4-beta doesn't handle objects containing circular references well.

Steps to reproduce:

  1. Define a ng-options with ng-model. At least one of the items in the source array must be a recursive object (such as a Breeze entity).
  2. Select the recursive object.
  3. Uncaught RangeError: Maximum call stack size exceeded is printed on the console.

http://jsfiddle.net/dubejf/tn102eh8/ (using 1.4.0-beta6)

The watcher of ngModel, set by the ngOptionsDirective, compares the selected item with the last value by deep equality (using angular.equals). When a recursive object is selected, this causes the stack to blow up (as described in #7724).

Using track by doesn't alleviate the problem.

This error is not reproducible in Angular 1.3.
@mgbee8
Copy link

mgbee8 commented Mar 25, 2015

I can confirm and reproduce this error with Breeze entities, it was working correctly in beta5 but beta6 displays this behavior

@dubejf
Copy link
Author

dubejf commented Mar 25, 2015

You're right, it was broken by this commit: 6a03ca2

@Narretz Narretz added this to the 1.4.0-rc.0 / 1.3.17 milestone Mar 26, 2015
@Narretz Narretz mentioned this issue Mar 28, 2015
Closed
@booleanbetrayal
Copy link
Contributor

#11448 offers a way to work around this issue, provided you aren't trying to use track by

@petebacondarwin
Copy link
Contributor

Having landed #11448 I can see that this bug appears to be fixed: http://jsfiddle.net/L9vqc9ew/

@dubejf
Copy link
Author

dubejf commented Apr 11, 2015

Yes, this bug is fixed in 1.4.0-rc.0, as long as you are not using track by. track by with cyclic objects still gives an error, but that regression seems less important.

In any case, with the recent performance optimizations to ngOptions (identity comparison, no copies), is it fair to say to track by should be avoided when using objects with ngOptions? As opposed to ngRepeat, where the guideline appears to be to use track by whenever possible.

@pranaydutta89
Copy link

👍

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Apr 20, 2015
@lgalfaso
fix(equals): handle objects with circular references
daaed2e 
Make `angular.equals` handle circular references.

Closes angular#11372
@lgalfaso lgalfaso mentioned this issue Apr 20, 2015
Closed
@lgalfaso lgalfaso self-assigned this Apr 20, 2015
@chirayuk chirayuk modified the milestones: 1.4.0-rc.1, 1.4.0-rc.2 Apr 24, 2015
@petebacondarwin
Copy link
Contributor

I think this is a better fix #11743

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Apr 28, 2015
@petebacondarwin
fix(ngOptions): use watchCollection not deep watch of ngModel
1f9163e 
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 was not documented as supporting such a situation. The
documentation has been updated to clarify this behaviour.

Closes angular#11372
Closes angular#11653
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Apr 29, 2015
@petebacondarwin
fix(ngOptions): use watchCollection not deep watch of ngModel
e6ab5ab 
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
@booleanbetrayal
Copy link
Contributor

@petebacondarwin - I think the select documentation also needs to be updated as well -

angular.js/src/ng/directive/select.js

Lines 131 to 133 in abf59c2

* **Note:** By default, `ngModel` compares by reference, not value. This is important when binding to an
* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/). When using `track by`
* in an `ngOptions` expression, however, deep equality checks will be performed.

@petebacondarwin
Copy link
Contributor

Yes that block is not valid in that directive anyway, since it references ngOptions

@petebacondarwin
Copy link
Contributor

@booleanbetrayal - Would you like to put together a PR with it removed/changed?

@booleanbetrayal
Copy link
Contributor

Sure thing

@booleanbetrayal
Copy link
Contributor

@petebacondarwin - #11758

netman92 pushed a commit to netman92/angular.js that referenced this issue Aug 8, 2015
@petebacondarwin @netman92
fix(ngOptions): use watchCollection not deep watch of ngModel
0222b2c 
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
@rasmusvhansen rasmusvhansen mentioned this issue Nov 25, 2015
Open
@Malczewski Malczewski mentioned this issue Nov 7, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@lgalfaso lgalfaso

Labels
component: forms severity: regression type: bug
Projects
None yet
Milestone
1.4.0-rc.2
Development

Successfully merging a pull request may close this issue.

fix(equals): handle objects with circular references lgalfaso/angular.js
9 participants
@petebacondarwin @chirayuk @matsko @lgalfaso @mgbee8 @booleanbetrayal @Narretz @pranaydutta89 @dubejf