fix(copy): preserve prototype chain when copying object #5063

gentooboontoo · 2013-11-21T08:57:30Z

So far, angular.copy was copying all properties including those from
prototype chain and was losing the whole prototype chain (except for Date,
Regexp, and Array).

Deep copy should exclude properties from the prototype chain because it
is useless to do so. When modified, properties from prototype chain are
overwritten on the object itself and will be deeply copied then.

Moreover, preserving prototype chain allows instanceof operator to be
consistent between the source object and the copy.
Before this change,

var Foo = function() {};
var foo = new Foo();
var fooCopy = angular.copy(foo);
foo instanceof Foo; // => true
fooCopy instanceof Foo; // => false

Now,

foo instanceof Foo; // => true
fooCopy instanceof Foo; // => true

The new behaviour is useful when using $http transformResponse. When
receiving JSON data, we could transform it and instantiate real object
"types" from it. The transformed response is always copied by Angular.
The old behaviour was losing the whole prototype chain and broke all
"types" from third-party libraries depending on instanceof.

This PR is somewhat related to issues #4996, #3767 and #1427.

petebacondarwin · 2013-11-21T16:39:55Z

Can you provide an explicit example of the use case that motivates this change?

gentooboontoo · 2013-11-21T17:39:25Z

I have instances with "typed" properties I need to persist with JSON format.

For instance, I could have the following object whose one of its properties is an instance of Qty from my library gentooboontoo/js-quantities (but it could be an instance from moment.js library):

var record = {
    ...
    volume: new Qty('3 l')
    ...
};

record.volume instanceof Qty; // => true

This record is persisted in JSON with its type inlined because the property value is not JSON native type:

{
    ...
    "volume": {
      "value": "3 l",
      "type": "quantity"
    }
    ...
};

I choose to override default transformResponse from $resource to deserialize this record and properly instantiate record properties (I hope it is the good way to do it).

transformResponse returns the original deserialized version of the persisted record as expected:

var recordFromTransformResponse = {
    ...
    volume: new Qty('3 l')
    ...
};

recordFromTransformResponse.volume instanceof Qty; // => true

Unfortunately, Angular resource make a copy of that deserialized object and the final object retrieved is:

var recordFromResource = Record.get(...);
...
// Surprisingly it is false
recordFromResource.volume instanceof Qty; // => false

Some code on my side needs properties to respond correctly to instanceof operator (whether or not the record has just been created by the app or received through $resource).

angular.copy removes the prototype chain of all nested properties not being a native type and copy every properties from the whole prototype chain on the object itself. I don't see any usefulness even with deep copy. If source object has "shared" properties through its prototype chain, its deep copy should share these properties through its prototype chain too (I could be wrong but it is still a real deep copy).

Am I wrong in my reasoning?

IgorMinar · 2013-12-07T10:57:50Z

I'm sorry, but I wasn't able to verify your 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 me 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.

gentooboontoo · 2013-12-07T12:44:03Z

I have just fixed the e-mail of all PR commits and I have signed the CLA with the right e-mail.

gentooboontoo · 2013-12-16T11:13:05Z

I have rebased the commits onto master and refactored them in two:

the first one related to the feature
a second one for IE8 polyfill (to be fixed or dropped depending on the target milestone if PR is accepted)

@petebacondarwin, I have also unpublished changes (based on this PR) to copy getters and setters that could fix #5085 but I not sure if they should be integrated here.

gentooboontoo · 2014-02-13T16:52:38Z

As Angular 1.3 discontinues IE8 and as I do not expect this PR to land on Angular 1.2x branch, I will remove failing IE8 tests in the very near future.

gentooboontoo · 2014-03-07T13:31:05Z

I have rebased the PR onto master and I have dropped IE8 support. All tests pass now.

caitp · 2014-04-09T22:48:44Z

src/Angular.js

@@ -770,7 +770,7 @@ function copy(source, destination){
      } else if (isRegExp(source)) {
        destination = new RegExp(source.source);
      } else if (isObject(source)) {
-        destination = copy(source, {});
+        destination = copy(source, Object.create(Object.getPrototypeOf(source)));


This line means that backporting this to 1.2.x is impossible or difficult.

I think we could get around this, though, using a strategy similar to what we have in the injector:

function Constructor() {} Constructor.prototype = isFunction(source.constructor) ? source.constructor.prototype : Object.prototype; destination = copy(source, new Constructor());

I'm not positive that would work, but I think it would. It might be a bit expensive, though.

gentooboontoo · 2014-04-10T07:21:10Z

Thank you very much @caitp for your comment. In the first version of this PR, I tried to support IE<=8 by creating the following shim function to support Object.create into Angular.js:

/**
 * Creates object through Object.create or polyfill for IE <= 8
 *
 * @private
 * @param {Object} proto - The object representing the prototype of the new object to create
 * @returns {Object} Newly created object with proto as prototype
 * */
var createObject = (function () {
  function Constructor() {}
  if(typeof Object.create === "function") {
    return Object.create;
  }
  else { // msie <= 8
    return function createObject(proto) {
      Constructor.prototype = proto;
      return new Constructor();
    };
  }
}());

As you can see, createObject is defined through an IIFE so function Constructor() is only declared once and typeof Object.create === "function" only evaluated once.
Then we just have to replace Object.create calls with createObject instead:

destination = copy(source, createObject(Object.getPrototypeOf(source)));

This way, it should not be too expensive. If needed, I could make a separate PR for this or try to re-include it here.

gentooboontoo · 2014-04-10T07:33:44Z

I have realized that there's also a problem for Object.getPrototypeOf too with IE8.

As a side note, I didn't expect this PR to land to 1.2.x branch. I found these shims too overkill just to support IE8 with 1.2.x. But if needed I could try to fix them.

caitp · 2014-04-10T09:06:42Z

You can get the prototype of the constructor via the cobstructor property, if one exists. John Resig shows a polyfill at http://ejohn.org/blog/objectgetprototypeof/, which despite the caveats, should generally work "well enough'

shaneneuerburg · 2014-04-28T18:05:22Z

Thanks for the fix @gentooboontoo. This resolved an issue I was having using moment.js with angular. Hopefully the PR gets merged

leider · 2014-05-27T07:58:14Z

Hi, I am having the same issues with moment.js. I would highly appreciate integrating it. Can 100% confirm @shaneneuerburg It fixed my issue as well

etodanik · 2014-06-10T13:38:36Z

I'm also having trouble with that same momentjs issue. This would be pretty handy. From what I gather this isn't compatible with the latest 1.2.x branch? Would anyone know the way to adapt it?

gentooboontoo · 2014-06-10T16:24:09Z

@caitp @petebacondarwin I have rebased the PR onto master branch (the PR was outdated by some changes). I think this PR targets v1.3.x branch although it works with 1.2.x branch and non IE8 browsers.

If needed, I could re-add IE8 support I removed earlier (and fixed according to previous @caitp's comment) but it would be a little bit ugly as v1.3 is approaching.

etodanik · 2014-06-11T16:13:14Z

I applied this patch and it solved a very annoying problem with $watch on
moment.js.

It would be great to see this merged.

On Tue, Jun 10, 2014 at 7:25 PM, Julien Sanchez notifications@github.com
wrote:

@caitp https://github.com/caitp @petebacondarwin
https://github.com/petebacondarwin I have rebased the PR onto master
branch (the PR was outdated by some changes). I think this PR targets
v1.3.x branch although it works with 1.2.x branch and non IE8 browsers.

If needed, I could re-add IE8 support I removed earlier (and fixed
according to previous @caitp https://github.com/caitp's comment) but it
would be a little bit ugly as v1.3 is approaching.

—
Reply to this email directly or view it on GitHub
#5063 (comment).

myitcv · 2014-06-16T11:55:12Z

+1 (given #7846)

myitcv · 2014-06-27T07:29:51Z

Are there plans to include this in v1.3?

petebacondarwin · 2014-06-27T12:11:26Z

@lgalfaso - do you have a view on this? It seems like a reasonable change.

So far, angular.copy was copying all properties including those from prototype chain and was losing the whole prototype chain (except for Date, Regexp, and Array). Deep copy should exclude properties from the prototype chain because it is useless to do so. When modified, properties from prototype chain are overwritten on the object itself and will be deeply copied then. Moreover, preserving prototype chain allows instanceof operator to be consistent between the source object and the copy. Before this change, var Foo = function() {}; var foo = new Foo(); var fooCopy = angular.copy(foo); foo instanceof Foo; // => true fooCopy instanceof Foo; // => false Now, foo instanceof Foo; // => true fooCopy instanceof Foo; // => true The new behaviour is useful when using $http transformResponse. When receiving JSON data, we could transform it and instantiate real object "types" from it. The transformed response is always copied by Angular. The old behaviour was losing the whole prototype chain and broke all "types" from third-party libraries depending on instanceof.

gentooboontoo · 2014-06-27T12:57:35Z

Just rebased onto master.

IgorMinar · 2014-06-27T13:38:26Z

Looks reasonable to me. We should make a note about the breaking change (the new behavior is different) and incompatibility with ie8.

caitp · 2014-06-27T13:42:33Z

We should make a note about the breaking change (the new behavior is different) and incompatibility with ie8.

If we want to backport the fix into 1.2.x, this form of Object.create can be polyfilled, and __proto__ will work in IE8

IgorMinar · 2014-06-27T13:44:02Z

We are changing the behavior. So we should not backport this.

On Fri, Jun 27, 2014, 6:42 AM, Caitlin Potter notifications@github.com
wrote:

We should make a note about the breaking change (the new behavior is
different) and incompatibility with ie8.

If we want to backport the fix into 1.2.x, this form of Object.create can
be polyfilled, and proto will work in IE8

—
Reply to this email directly or view it on GitHub
#5063 (comment).

caitp · 2014-06-27T13:45:45Z

Think of the statement as a note for the next person to come along saying "I want the new features of 1.3, but I need to support IE8, what can I do" =)

IgorMinar · 2014-06-27T13:57:59Z

Maintain a patch. It's not ideal but you are supporting obsolete browser
already so you have bigger issues.
On Jun 27, 2014 6:45 AM, "Caitlin Potter" notifications@github.com wrote:

Think of the statement as a note for the next person to come along saying
"I want the new features of 1.3, but I need to support IE8, what can I do"
=)

—
Reply to this email directly or view it on GitHub
#5063 (comment).

gentooboontoo · 2014-06-27T15:47:35Z

I have initially made a branch with missing IE8 polyfills but it made the change ugly and clumsy (especially in branch 1.3 which does not support IE8).
I could try to put them back if asked but a library like es-shims/es5-shim could better help those who want to use missing ES5 features from IE8.

caitp · 2014-06-27T15:55:48Z

@gentooboontoo that's fine, don't worry about it. We aren't testing IE8 so it's not a big deal to not include such fixes here, since we'll never know if they break or not

lgalfaso · 2014-06-27T16:32:06Z

@petebacondarwin for 1.3.x, I think this is a no-brainer that we should land this. For 1.2.x I would not back port it

leider · 2014-06-27T16:50:59Z

what stops you collaborators from pulling?

caitp · 2014-06-27T16:54:38Z

@leider nothing whatsoever --- I think Pete is merging this one

gentooboontoo · 2014-06-30T13:30:18Z

Thanks a lot @petebacondarwin and @caitp.

myitcv · 2014-06-30T18:33:01Z

Thanks from me too.

However this has broken something for me - noted here. Thoughts?

Not sure whether comments on commits get to the right people... hence the 'repost'

gentooboontoo · 2014-07-01T15:05:00Z

I have made another PR (#8034) improving this one preserving own property descriptors and including own non-enumerable properties.

So far, angular.copy was copying all properties including those from prototype chain and was losing the whole prototype chain (except for Date, Regexp, and Array). Deep copy should exclude properties from the prototype chain because it is useless to do so. When modified, properties from prototype chain are overwritten on the object itself and will be deeply copied then. Moreover, preserving prototype chain allows instanceof operator to be consistent between the source object and the copy. Before this change, var Foo = function() {}; var foo = new Foo(); var fooCopy = angular.copy(foo); foo instanceof Foo; // => true fooCopy instanceof Foo; // => false Now, foo instanceof Foo; // => true fooCopy instanceof Foo; // => true The new behaviour is useful when using $http transformResponse. When receiving JSON data, we could transform it and instantiate real object "types" from it. The transformed response is always copied by Angular. The old behaviour was losing the whole prototype chain and broke all "types" from third-party libraries depending on instanceof. Closes angular#5063 Closes angular#3767 Closes angular#4996 BREAKING CHANGE: This changes `angular.copy` so that it applies the prototype of the original object to the copied object. Previously, `angular.copy` would copy properties of the original object's prototype chain directly onto the copied object. This means that if you iterate over only the copied object's `hasOwnProperty` properties, it will no longer contain the properties from the prototype. This is actually much more reasonable behaviour and it is unlikely that applications are actually relying on this. If this behaviour is relied upon, in an app, then one should simply iterate over all the properties on the object (and its inherited properties) and not filter them with `hasOwnProperty`. **Be aware that this change also uses a feature that is not compatible with IE8.** If you need this to work on IE8 then you would need to provide a polyfill for `Object.create` and `Object.getPrototypeOf`.

petebacondarwin mentioned this pull request Nov 22, 2013

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

Closed

dobladez mentioned this pull request Jan 28, 2014

moment.js and scope.$watch when objectEquality is true #4996

Closed

caitp reviewed Apr 9, 2014
View reviewed changes

etodanik mentioned this pull request Jun 10, 2014

momentjs causing problems due to bug with $watch in angular.js esvit/ng-table#344

Closed

rodyhaddad added component: misc core and removed needs: review labels Jun 10, 2014

lgalfaso mentioned this pull request Jun 16, 2014

Use source object prototype in object copy #7846

Closed

gentooboontoo changed the title ~~Preserve prototype chaining when copying object~~ Preserve prototype chain when copying object Jun 27, 2014

gentooboontoo changed the title ~~Preserve prototype chain when copying object~~ fix(copy): preserve prototype chain when copying object Jun 27, 2014

petebacondarwin closed this in b59b04f Jun 30, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(copy): preserve prototype chain when copying object #5063

fix(copy): preserve prototype chain when copying object #5063

gentooboontoo commented Nov 21, 2013

petebacondarwin commented Nov 21, 2013

gentooboontoo commented Nov 21, 2013

IgorMinar commented Dec 7, 2013

gentooboontoo commented Dec 7, 2013

gentooboontoo commented Dec 16, 2013

gentooboontoo commented Feb 13, 2014

gentooboontoo commented Mar 7, 2014

caitp Apr 9, 2014

gentooboontoo commented Apr 10, 2014

gentooboontoo commented Apr 10, 2014

caitp commented Apr 10, 2014

shaneneuerburg commented Apr 28, 2014

leider commented May 27, 2014

etodanik commented Jun 10, 2014

gentooboontoo commented Jun 10, 2014

etodanik commented Jun 11, 2014

myitcv commented Jun 16, 2014

myitcv commented Jun 27, 2014

petebacondarwin commented Jun 27, 2014

gentooboontoo commented Jun 27, 2014

IgorMinar commented Jun 27, 2014

caitp commented Jun 27, 2014

IgorMinar commented Jun 27, 2014

caitp commented Jun 27, 2014

IgorMinar commented Jun 27, 2014

gentooboontoo commented Jun 27, 2014

caitp commented Jun 27, 2014

lgalfaso commented Jun 27, 2014

leider commented Jun 27, 2014

caitp commented Jun 27, 2014

gentooboontoo commented Jun 30, 2014

myitcv commented Jun 30, 2014

gentooboontoo commented Jul 1, 2014

fix(copy): preserve prototype chain when copying object #5063

fix(copy): preserve prototype chain when copying object #5063

Conversation

gentooboontoo commented Nov 21, 2013

petebacondarwin commented Nov 21, 2013

gentooboontoo commented Nov 21, 2013

IgorMinar commented Dec 7, 2013

gentooboontoo commented Dec 7, 2013

gentooboontoo commented Dec 16, 2013

gentooboontoo commented Feb 13, 2014

gentooboontoo commented Mar 7, 2014

caitp Apr 9, 2014

Choose a reason for hiding this comment

gentooboontoo commented Apr 10, 2014

gentooboontoo commented Apr 10, 2014

caitp commented Apr 10, 2014

shaneneuerburg commented Apr 28, 2014

leider commented May 27, 2014

etodanik commented Jun 10, 2014

gentooboontoo commented Jun 10, 2014

etodanik commented Jun 11, 2014

myitcv commented Jun 16, 2014

myitcv commented Jun 27, 2014

petebacondarwin commented Jun 27, 2014

gentooboontoo commented Jun 27, 2014

IgorMinar commented Jun 27, 2014

caitp commented Jun 27, 2014

IgorMinar commented Jun 27, 2014

caitp commented Jun 27, 2014

IgorMinar commented Jun 27, 2014

gentooboontoo commented Jun 27, 2014

caitp commented Jun 27, 2014

lgalfaso commented Jun 27, 2014

leider commented Jun 27, 2014

caitp commented Jun 27, 2014

gentooboontoo commented Jun 30, 2014

myitcv commented Jun 30, 2014

gentooboontoo commented Jul 1, 2014