making the equals method work when the obj graph has circular refs

[andersonbd1]

No description provided.

[mary-poppins]

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[andersonbd1]

Sorry about that, Mary. I haven't contributed much to github and nothing at all to angular, so I completely missed that. I signed the CLA. Let me know if you need anything else.

[andersonbd1]

I should add that this fix is relevant when you start using something like jsog (which is what led me to make this fix)

[caitp]

we've talked about supporting circular references in equals() before --- #7839 for example (although there are a lot of similar cases on the issue tracker).

The general idea is that it's just really expensive, so we probably don't want to do it =\

[caitp]

I don't really expect this to be fixable in core, sadly --- it would be nice if there was a better way to do this.

[caitp]

I'm going to throw this in the ice box for now, /cc @IgorMinar in case he changes his mind about this. But I wouldn't expect this to go anywhere :(

[jbedard]

Won't visitedObjArr[obj] always be equivalent to visitedObjArr['[object Object]']. Seems pretty broken...

[andersonbd1]

I took a look at the other pull request you referenced (7839). My commit should be better performant because it uses an associative array (which I assume is indexed by object identity). However, in looking at 7839, I noticed I have a logical error in mine. I think if you changed 7839 to use an associative array instead of an ordered array, then the performance concern would go away (sorry if I'm using the wrong terminology - js isn't my sweet spot). Would this fix be considered if I were to submit that change? Is there a way to validate if this is actually a performance issue or whether it's just a hunch?

[caitp]

(which I assume is indexed by object identity).

nope. We have the option of using ES6 WeakMaps for this, but the problem is that we are supporting browsers that don't all ship WeakMaps currently. @jbedard is correct that names will always be either strings or symbols (in browsers which support symbols). The HashMap algorithm in the tree was one option, but we just decided not to do it, it kind of sucks since it tries to add a hash key to the object if there was none before.

[andersonbd1]

ok thanks for the responses. It sounds like this has already been thought through pretty well. A couple more questions.

1. what sucks about adding a hash key to the object? that seems like it wouldn't add too much overhead

2. does angular have a way to get extended functionality (like this) if you don't care about old/crappy browsers? If I decide to use a variant of this fix in my app, is there a kosher way to do so or will I just need to maintain that on my own?

[caitp]

1. well, sometimes people aren't happy discovering new properties ended up in their objects :( --- If we used Symbol properties, it would be a lot less visible to them, though. Unfortunately, that limits us to browsers with non-polyfilled Symbols, which is like at-most 35% (well, on sites using angular it's probably a lot higher than that, but yeah, FF isn't shipping Symbols in stable yet ._.)

2. if we used angular.equals() in core, then it would work. But in core we just use equals(), so even if you override angular.equals(), it would invoke the original function. The only way to make this work is to patch the library for your own uses, which is perfectly legal and fits the license for 1.x just fine.

[andersonbd1]

ok - I'll use a patched version for my own use. Thanks, much. I'll update my commit just in case anyone else wants to use it.

[andersonbd1]

and here's the implementation with WeakMap that I'll be using as a patched version. Just posting back here for reference.

andersonbd1@5696869

[l0co]

This https://gist.github.com/l0co/e029063b91c2a78c3889 does the job as well. This is a very raw hack I needed to add to angular to make it working with object trees containing circular references. And we really need circular references in this project because we keep references to the same entities as the same JS objects to allow them to be quickly updated from asynchronous events.

It just assumes that no one uses deeper object structure than N levels (it 'd be rather a madness) and if we on the such level it needs to be circular reference, and thus the object has been already visited and apparently equals() is true. This is as fast as incrementing/decrementing integers and checking simple arithmetic condition. I can't imagine what can be faster.

It'd be a good idea to allow to configure for angular this thing - max tree level to scan, and it can have zero performance impacts if you use original function by default, and only switch implementation to similar to my on exact request from the client code, that it needs to limit this scanning level.