Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

angular.copy not copying accessors (getters/setters) #5085

Closed
chtulhu6662000 opened this issue Nov 22, 2013 · 11 comments
Closed

angular.copy not copying accessors (getters/setters) #5085

chtulhu6662000 opened this issue Nov 22, 2013 · 11 comments

Comments

@chtulhu6662000
Copy link

It seems like angular.copy doesn't re-create accessors (getter/setter properties) when cloning, which leads to a break in behavior.

For instance:

    var template = {
        data: {label: 'new node'},

        get label() {return this.data.label;},
        set label(value) {  this.data.label = value;}
    };

    var test = angular.copy(template);
    test.label = 'not working';
    console.log(test.data.label); // Should be 'not working', but instead is 'new node'
@petebacondarwin
Copy link
Contributor

There are a number of issues related to angular.copy: #5063 and #2618.

I guess we either need to make it much more powerful or state that it is really only supposed to be used for simple copies of plain objects and expect developers to use a 3rd party copy library or write their own if they want this kind of stuff. @IgorMinar, @mhevery and @vojtajina - what do you think?

@gentooboontoo
Copy link
Contributor

As reported in #5063, angular.copy is used by $resource and currently prevents instanciated objects in transformResponse to preserve their prototype chain.
If ever angular.copy will be meant to simple copies only, should $resource keep using it? PR #5063 preserving prototype chain does not break current deep copy behaviour of Angular (let alone plain objects it is supposed to copy).

Concerning this issue, unfortunately it does not seem to have standard solution to copy setters and getters.

@trisys3
Copy link

trisys3 commented Aug 19, 2015

It seems that if you specify the property as enumerable when defining it, it gets copied over. This makes sense, as loops are where the actual copying takes place. I'm not sure how you could do this without loops, but if there is a getter or setter you want to copy to a new object, you should probably make it enumerable anyway.

@programmist
Copy link

Using enumerable doesn't really work either. The angular.copy function only copies over a snapshot of the properties/values, but the property definitions are destroyed in the process. This probably isn't an issue if your getters simply return a discrete value. However, if they are doing anything more complex such as returning the result of an expression or referring to another internal object then angular.copy won't work.

@trisys3
Copy link

trisys3 commented Nov 5, 2015

I think you are right. It looks like the solution is to copy all the properties as well as their descriptors, using for example a for...in loop with getOwnPropertyDescriptor and defineProperty, but this would be slow. The difference is still potentially an issue, but the speed difference involved probably requires either another argument or a whole other function entirely.

@timostamm
Copy link

Just some food for thought:

To my knowledge, it is impossible to create an exact copy of arbitrary objects in Javascript. (Edit: wording)

Copying getters and setters does not resolve this goal, and complicates the issue for users that expect an exact copy. Consider:

function Foo() {
  var privateBazVal = 123; // this is our private state
  Object.defineProperty(this, 'baz', {
    get : function () {
      return privateBazVal;
    },
    set : function ( val ) {
      privateBazVal = val;
    }, 
    enumerable: true
  });
}

Now let's create an instance and copy it:

var foo = new Foo();
foo.baz === 123; // true
var fooCopy = copy( foo );
fooCopy.baz = 777;
foo.baz === 123; // false

The objects still share state. (Yes, it is a very simple example.)

Other programming languages have tackled that problem:
https://en.wikipedia.org/wiki/Clone_(Java_method)
http://php.net/manual/en/language.oop5.cloning.php

They delegate to the object via a copy-method. Domain-objects can decide how to a copy of themselves should be created.

These languages have standardized this, though.

@trisys3
Copy link

trisys3 commented Feb 26, 2016

Is there anything in ES2015-2016 with regards to delegating to a copy-method? I know there are proxies and annotations, but I do not know if they allow this kind of behavior directly. Maybe you could make an annotation or proxy (or an annotation on a proxy object) that makes the getter(s) delegate to another function on copy, or something similar.

@programmist
Copy link

My use case may have been different than the issue author's (I don't recall). I wanted to copy both the property definitions and the state. I'm don't know about a clone function in ES6+, but you can use Object.assign to achieve a similar result.

Modifying @timostamm's example:

var foo = new Foo();
foo.baz === 123; // true
var fooCopy = Object.assign(new Foo(), foo); // Construct new object, then assign
fooCopy.baz = 777;
foo.baz === 123; // true

@timostamm
Copy link

Object.assign requires the properties to be enumerable, which a 3rd party library might not do.

It is also unable to recreate constructor-arguments, which means it cannot copy Foo if it was constructed with a custom initial value. Slightly modified example:

function Foo( initialBazVal ) {
  var privateBazVal = initialBazVal;
  ...
}

new Foo(999) cannot be cloned using Object.assign without knowledge about the implementation of Foo.

Other examples:

  • Suppose you have a reference to the currently logged-in user in your object. Should the copied object really point to a different user instance?
  • Let's say you have a reference to a angular service in your object. Services are singletons. Should there be two instances after copying?
  • You have event listeners bound to the object. Should they fire for the copy as well?
  • For some reason you have a reference to the Window object. This cannot be copied.

These examples may seem far-fetched, but they will happen sometime, somewhere, which is why angular cannot provide a catch-all copy function.

I see two approaches to make it generic:

a) make it possible to override the copy-function

I am not sure how this would work. If one piece of software requires a specific implementation and another piece of software requires a different implementation, how would you merge the functionality?

b) honor a magic $$copy method

This is a proven concept and should work out without noticable performance degradation.
Because angular copies watched scope objects to compare them later, $$copy would lift some restrictions on change watching. Bonus: a companion $$equals method honored in angular.equals would fit well to that.

Foo would have to implement:

this.$$copy = function( copyFn ){
  return new Foo( privateBazVal );
};

And possibly:

this.$$equals = function( other ){
  return other instanceof Foo && other.baz === privateBazVal;
};

@programmist
Copy link

@timostamm: You bring up good points. There are indeed several 'copy' scenarios that the Object.assign work-around I demonstrated does not cover. I probably should have been more clear that it was meant as a work-around for the original author's use case, not a catch-all cloning solution.

@tiansh
Copy link

tiansh commented Aug 10, 2017

A very dirty hack works on 1.5 / 1.6:

const copiableProperties = function (object, details) {
  Object.defineProperties(object, details);
  object.cloneNode = (function (cloneNode) {
    return function () {
      const that = angular.copy(Object.assign({}, this, { cloneNode }));
      Object.defineProperties(that, details);
      that.cloneNode = angular.copy(this.cloneNode);
      return that;
    };
  }(object.cloneNode));
};

fiddle

Narretz added a commit that referenced this issue Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants