Make angular.equals support objects with circular references

[myitcv]

Per #7724

[myitcv]

@rodyhaddad - anything more you need from me on this?

[IgorMinar]

This is too expensive and I don't think that we can make it much cheaper.

Deep watching is already expensive as is and adding more complexity to it to cover corner cases doesn't make much sense to me.

One alternative option I came up with was to add another flag to $watch that would mean "support circular references".

In the real world, we don't see any significant apps needing this feature so unless we do, I'd prefer not to support it.

[IgorMinar]

thanks for the PR though... I'm sorry about resolving it like this..

[jordanms]

This is already closed but I have few comments.

This bug destroys the application performance because all the recursive calls take lot of time before throwing _RangeError: Maximum call stack size exceeded_. Keeping track of which objects have already been compared is not that expensive that would compromise the application performance. Many APIs do that and have very good performance.

Circular reference (or bidirectional relationship) _is not_ corner case. If developer applies good OO design, soon or later one will use this tool. All of these, but not only, data structure can be implemented using circular reference: list, tree, map and set.

One can argue that those structures are not common in JS, but day after day more and more RESTful application are built and they are communicating with back-end servers which are developed in Java, .Net and other languages which heavily uses databases and bidirectional relationships.

Last but not least, Angularjs is a base framework and should not dictate how developers design their code.

[tjad]

Absolutely! I've been running into this issue for several months now, and until today I've kept it on the backlog. The time has come, only to discover that the angular community doesn't seem to hold the feature of true object oriented programming at heart, the core of any object oriented language, the object model, which by nature contains circular referencing.

[tjad]

@IgorMinar In fact, I've been doing some research in terms of the performance hit you speak of, and am unable to understand the significance that you find. I am even at this point willing to say that the performance would be faster tracking the object model comparisons being used as with a solution similar to that suggested in #9762

I understand that that this solution may reduce / limit the number of browsers that are supported, but that is a different concern - not one due to performance.

The solution uses weakmaps (and probably should utilize weak sets). Good WeakMap implementations will have search times of O(1), which in performance can almost be disregarded, but with the introduction of using these structures 2 extra searches within them will be made. On the other hand we have to consider memory. It too will not have a huge performance impact as the values stored within these data structures will be simple integers - the object id references.

Finally this brings us to the last problem, how well these data structures are supported by the browsers Angular is provided for. Again this is not a problem as Angular could provide their own implementation of the data structures - just as they've provided their own equals() method-, until more traction on ES6 is present.

Please let me know your thoughts

[gkalpak]

#10096 will provide a way to solve this as well.

[jordanms]

This is a very complicated way how to solve a simple problem and will force every developer to implement their own solution when it should be standard.

[jordanms]

@gkalpak this solution would not solve my problem, for example. I'm using a third party plugin (material design) which uses angular.equals. So a direct fix for this problem still needed to solve such kind of dependency.

[reda-alaoui]

We are unable to use https://github.com/jsog/jsog because angular doesn't deal well with circular references.
Support of circular reference would be great for us.

[tjad]

@jordanms As far as I understand the solution, you would be able to replace the default functionality of Angular.equals with your own, hence the solution they are providing should be very robust.

@gkalpak That said though, I am still in favour of what @jordanms is saying. That this should be solved at a fundamental level.

I do not agree at all with the excuse that this is a major performance hit as per #7839 (comment)
Rather, I believe that it is a lack of interest in change at the fundamental level. I believe this because it seems that Angular was initially built to deal with scalar values, hence the lack of support for object models (including with circular references). This came to me as I saw angular only at a later stage started supporting Map/List objects with which should have given the inclusion of proper support for objects and circular references.
I.E Angular was built for scalar values and the change to use proper objects for everything was either never really considered or was naively fought against, and it seems silly. OO gives a gain in performance with regards to many aspects. (largely including memory)

[jordanms]

@tjad I understood from the initial request that they want to implement what, in Java, we call Comparator to be used with $watch. However, I saw that the implementation would replace the default equals as you said, what I think is not a good idea. The concept of the Comparator is that a special equals function can be provided for a specific object (the watched object) in a specific context (the $watch). For all other objects and contexts, the default equals function should be used. If this approach is used, the default implementation for equals will not change and the support for CR still needed. Even if the implementation of #10096 goes in the direction it is right now (replacing the default equals implementation), changing equals implementation still needed for the reasons you explained.