fix(equals): handle objects with circular references

[lgalfaso]

Make angular.equals handle circular references.

Closes #11372

[campersau]

This might be faster (not tested):

var index = 0, length = cache.length;
while (index < length) {
  index = cache.indexOf(o1, index);
  if (index == -1) break;
  if (cache[index + 1] === o2) return true;
}
return false;

[gkalpak]

This is not safe as is (e.g. index has to be an even number).

[gkalpak]

I was under the impression that "we" had decided against supporting circular references (due to being expensive and not too common). E.g. #7724/#7839 or #9762.

Have "we" changed our mind ?

[lgalfaso]

@gkalpak If we were talking about only making equals support circular references, then I would say no. Now, #11372 is a valid (but somehow uncommon) use case. If there was another way to fix #11372 without making equals support circular references, then I think that would be best.

This PR is just to open the discussion if this is the approach to fix #11372

[gkalpak]

#11372 has been fixed (as long as you don't use track by).
Do we have a usecase that makes it necessary to use track by with ngOptions and objects with circular references ?

If it turns out we do want to support track by with circular references, maybe we should have two modes for equals: with and without support for circular references, so other paths (e.g. deep watches) do not get affected.
Or maybe the performance hit isn't really noticable...

[lgalfaso]

A simple option would be to use `cache` only if present and send it in this specific case from ngOptions

[jbedard]

This will effect performance, similar to copy (removing the circular reference part makes it much faster).

Using an ES6 Map instead of array is much faster, but a shimmed Map is slower. Would it be worth creating a Map shim now that both equals and copy can use it?

[lgalfaso]

@jbedard I am not happy with this solution. This may have performance implications just as with copy that are very noticeable. That said, I do not like the idea of a polyfill of Map/Set and torn between an optional parameter that would make equals circular references aware and wont fix.

[jbedard]

Yes it will have noticeable performance implications. I think you're right that it would have to be optional just to avoid effecting performance of existing calls (like in the digest loop!).

What don't you like about the Map shim though?

[lgalfaso]

What don't you like about the Map shim though?

It would make Angular execution less predictable. The reason being that if the user uses a library that provides a polyfill (say they use babel), then Angular would use it if this is defined before Angular. This is, Angular behavior would depend on the order libraries are loaded.

[jbedard]

I thought that extra uncertainty might be worth it since most browsers (all the latest versions) have Map.

[lgalfaso]

@jbedard if you are ok, I think that a new issue would be a better place to discuss about a potential Map/Set shim.

With the entire discussion, I am leaning towards wont fix, but would like to know what others think

[gkalpak]

(Shim considerations aside) I would be OK with an "opt-in" mode supporting circular references (e.g. as @lgalfaso suggested, only when explicitly passing a cache). And using this mode for ngOptions with track by only (which should have no effect on the performance of the rest of the code).
But, since ngOptions+track by+circular objects is a corner(-ish) case, I could go with won't fix as well :)

But it would be interesting to hear what others think.

[lgalfaso]

@petebacondarwin WDYT?
The options would be:

• The patch as is
• The patch with the modification at fix(equals): handle objects with circular references #11653 (comment)
• Wont fix
• Something else

[petebacondarwin]

I am nervous about supporting cyclic equality checks. I wonder if the real fix is to use $watchCollection in ngOptions rather than a deep watch. I can't remember why we are not doing this already.

[lgalfaso]

@petebacondarwin you are not the only one unease about this. $watchCollection would not work as we are comparing a copy of the original value. If the instance is the same, then equals already short-circuit this case :(

BTW, I would be ok in leaving this as a known issue for 1.4.0

[petebacondarwin]

How about this instead: #11743

[petebacondarwin]

I think that my original unit test was too strong and not real life

[lgalfaso]

I think that #11743 is a lot cleaner, I just do not know if this would break other use cases and expectations. It somehow feels like we are moving the issue from one place to another (that, too would imply a known issue in the release notes).

Note: I see #11743 as a POC that would also need to change the comments as what we are doing and what we claim we are doing, and what the test claim to check and what it test, do not match.

[petebacondarwin]

#11743 is more inline with what happens in ngRepeat, which also only watches the collection and not a deep watch. The original issue that triggered the deep watch still works with $watchCollection.

I think if we made this very clear, i.e. that we only watch the file level of the model and that people cannot change levels below and expect the model to update accordingly, we should be fine.

[lgalfaso]

@petebacondarwin SGTM, but this needs to be documented. Will close this PR in favor of #11743

[petebacondarwin]

I am working to improve that PR now