fix(angular.copy): support circular references

[rodyhaddad]

This PR is in response of #7592

I broke it into two parts:

1. Making angular.copy support circular references. This is useful for the digest loop and for $parse's bind-once feature. We're deeply copying user defined objects, which will make .copy hit the maximum call stack if it can't handle circular references.
2. Since .copy is now heavier, shallowCopy can take its place in many scenarios. But it needs to handle
   arrays and primitives to do the full job (mainly for ngClass)

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7618)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[rodyhaddad]

I'm not so sure about having the stack* parameters public.
Maybe a private version of .copy should be created and used internally.

[caitp]

I don't think it really hurts to expose them, we could separate it but all it would do is add an extra function call and make the code a bit bigger --- probably not worth it.

[IgorMinar]

+1 to what cait said. just not document them.

[caitp]

I feel like this is a potential breaking change, but lets see what happens :> it maybe won't break anyones applications

[caitp]

@IgorMinar is it out of the question to use $$hashKey for objects copied in this way? it would certainly be quicker than this, but it seems wrong to mess with peoples objects. Could do it and see if people complain, though

[IgorMinar]

we can't mess with the objects here.

[IgorMinar]

lgtm

[myitcv]

@rodyhaddad - thanks for raising this in response to #7592.

Per my latest comment on #7592, my stab at implementing this made angular.copy support circular references by default. I notice that yours requires explicit enabling via setting the third parameter to be true.

The side effect of it not being the default behaviour is that every caller needs to update (including places in core)

Am I missing something here? Seems like a safe, backwards compatible change, on the basis that any previous call to copy with a circular reference will have blown up.

[myitcv]

Sorry for the noise. To clarify my last point: I was trying to say that I think that making this enabled by default is "a safe, backwards compatible change...." Hence why my implementation was such.

[rodyhaddad]

@myitcv while enabling it by default is backward compatible, we put the support of circular references behind a flag for performance reasons. Most uses of angular.copy don't need it, so the extra lookup in the stackSource array is unnecessary.

We updated the $digest and the $parse bindonce to turn on the circular reference support when copying user defined values, which I believe will solve your initial problem.

And no worries about the noise, you asked for clarifications, so 👍

[caitp]

Actually, I think he's right --- since the only way to not be affected by circular references when using copy is to just use shallowCopy instead (which will never recursively copy anything).

So it probably should be the default behaviour, and everything that legitimately doesn't need to care should simply use shallowCopy

[rodyhaddad]

@caitp The issue is that other modules in the codebase use angular.copy.
These are ngCookies, ngMock, ngRoute and ngResource.
They don't need the circular reference support, and we don't want to expose angular.shallowCopy just for them.

[caitp]

@rodyhaddad angular.resource has its own implementation of shallowCopy (or rather, based on it), it's a pretty trivial routine, and is not difficult for modules to implement on their own if it's not made public

[myitcv]

@rodyhaddad - are the performance implications that severe? (I ask that without any reference to any numbers...)

If they are, then I would argue that we (and that's all users of angular.copy) should be looking at how to reduce the usage of angular.copy if possible.

My use case is, I believe, a sensible one - I angular.copy an object when I come to edit said object in a form. Very limited number of calls, user-driven (albeit indirectly).

The $digest call to angular.copy also can't be avoided, and it needs to support the handling of circular references because the developer can't turn it on/off as required. This is most likely to be the heaviest user of angular.copy given its position.

Unless you want the developer to flag on an object that circular reference support should be enabled. Via something like:

var x = {a: 123};
x.$copy_circular_reference_support = true;
var y = angular.copy(x);
...

with the corresponding code change in angular.copy itself. Whilst not pretty, it does address the dual issue of performance and callers of angular.copy (who could be third party library writers) forgetting to add the third parameter true

That's a rather long rambling argument in favour of making this be 'on' by default 👍

[myitcv]

On reflection, scrap that suggestion on the $copy_circular_reference_support - it's too messy.

I would bolster an earlier point I made to say that not only should use of angular.copy be limited where possible, use of functionality that depends on it should too, e.g. $watch (a pub/sub system can far more effectively replace this)

[rodyhaddad]

I talked more with @IgorMinar about this, what we will do is:

• The flag will be removed, making circular references always supported by angular.copy
• Other modules who use angular.copy can start using angular.extend({}, obj), which is the same as a shallowCopy, but just for objects
• angular.extend will be optimized for handling 2 arguments (it currently always expects a N amount of args)

Edit: hm, #2 doesn't really work, because angular.copy also deletes the old properties. angular.extend does not.
So we'll just remove the flag and keeps the rest as is.

[myitcv]

Thanks @rodyhaddad and @caitp