Proposal for angular.copy supporting circular references

[myitcv]

#2618 and various other posts have explored and explained why Angular current does not support the copying of objects with circular references. I believe this is an unnecessary restriction and so would like to make a proposal.

The proposal is simple. Modify angular.copy such that:

function copy(source, destination){
  if (isWindow(source) || isScope(source)) {
    throw ngMinErr('cpws',
      "Can't copy! Making copies of Window or Scope instances is not supported.");
  }

  // ***** ADD THESE LINES ****
  if (source && (source.$clone != null)) {
    return source.$clone(destination);
  }

  // .... 

(The choice of $clone as the function name is intentional)

This then allows the developer to explicitly add support on objects that contain circular references.

Thoughts?

[rodyhaddad]

While having that $clone does seem cool, I don't think it should be in the core.
In these situations, it's better to be explicit and invoke a clone function on the object:

var cloneBike = myBike.clone();
// or
var cloneBike = Vehicle.clone(myBike)

---

You can view the functions exposed on the angular object as utilities used internally by Angular, that get exposed for the user's convenience.
They're not there to solve the cases that Angular doesn't need. Other libraries can do that better (e.g. lodash)

[myitcv]

@rodyhaddad - precisely, the exact issue here is that angular.copy (along with angular.equals) IS used internally within the core. Indeed other Angular libraries use this exposed function as well.

The problem this presents is that any Angular application can only ever support objects with no circular references. This is the 'unnecessary restriction' I refer to in my original post.

So I'm really arguing for this change because I can't control all of the places where angular.copy is called.

This change is backwards compatible and puts the onus on the developer to 'get it right'

Very happy to raise a PR with tests etc if that helps.

[rodyhaddad]

@myitcv good call.
I talked with @IgorMinar about this. I'll work on a PR to have angular.copy support circular references on its own, without the need for the developer to use a .$clone.

It's good to note though that the js statement you linked to only gets called when objectEquality is turned on, which should be avoided whenever possible because of performance.

[myitcv]

@rodyhaddad - thanks. And thanks for the note on objectEquality - that's been forefront in my mind for a while. Not least because the use of $scope.$watch(.., .., true) is how I came across this issue in the first place.

Incidentally, I also asked this question about an error in the docs which I think is related. Thoughts on that?

[myitcv]

@rodyhaddad - raised a PR for the doc change

I also have a branch with a first cut at the changes required for angular.copy to support circular references.

[petebacondarwin]

I believe this is closed by #7618