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

Commit

Permalink
fix(Angular.copy): preserve prototype chain when copying objects
Browse files Browse the repository at this point in the history
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 #5063
Closes #3767
Closes #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`.
  • Loading branch information
gentooboontoo authored and petebacondarwin committed Jun 30, 2014
1 parent ffdde26 commit b59b04f
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 6 deletions.
15 changes: 9 additions & 6 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,8 @@ function copy(source, destination, stackSource, stackDest) {
} else if (isRegExp(source)) {
destination = new RegExp(source.source);
} else if (isObject(source)) {
destination = copy(source, {}, stackSource, stackDest);
var emptyObject = Object.create(Object.getPrototypeOf(source));
destination = copy(source, emptyObject, stackSource, stackDest);
}
}
} else {
Expand Down Expand Up @@ -807,12 +808,14 @@ function copy(source, destination, stackSource, stackDest) {
delete destination[key];
});
for ( var key in source) {
result = copy(source[key], null, stackSource, stackDest);
if (isObject(source[key])) {
stackSource.push(source[key]);
stackDest.push(result);
if(source.hasOwnProperty(key)) {
result = copy(source[key], null, stackSource, stackDest);
if (isObject(source[key])) {
stackSource.push(source[key]);
stackDest.push(result);
}
destination[key] = result;
}
destination[key] = result;
}
setHashKey(destination,h);
}
Expand Down
10 changes: 10 additions & 0 deletions test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ describe('angular', function() {
expect(copy([], arr)).toBe(arr);
});

it("should preserve prototype chaining", function() {
var GrandParentProto = {};
var ParentProto = Object.create(GrandParentProto);
var obj = Object.create(ParentProto);
expect(ParentProto.isPrototypeOf(copy(obj))).toBe(true);
expect(GrandParentProto.isPrototypeOf(copy(obj))).toBe(true);
var Foo = function() {};
expect(copy(new Foo()) instanceof Foo).toBe(true);
});

it("should copy Date", function() {
var date = new Date(123);
expect(copy(date) instanceof Date).toBeTruthy();
Expand Down

7 comments on commit b59b04f

@myitcv
Copy link
Contributor

@myitcv myitcv commented on b59b04f Jun 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this change to include the source.hasOwnProperty(key) check breaks properties that are defined via getters/setters. To demonstrate the problem outside of Angular (via CoffeeScript, although this is clearly entirely legit in Javascript), here is an example, code reproduced below:

class A
  constructor: ->
  Object.defineProperty @prototype, 'test', 
    get: -> @["_test"]
    set: (v) -> 
      Object.defineProperty @, '_test',
        writable: true
        enumerable: false
      @["_test"] = v
      console.log 'We just set test to ', v
    enumerable: true


a = new A
a.test = 5
console.log a.hasOwnProperty('_test')
console.log a.hasOwnProperty('test')

# output:
# We just set test to  5
# a.hasOwnProperty('_test') true
# a.hasOwnProperty('test') false # this is where things go wrong 

Per the docs for hasOwnProperty:

This method can be used to determine whether an object has the specified property as a direct property of that object

So a.hasOwnProperty('test') == false is as expected.

But this does break things for people who are defining properties as getters/setters (read: me at least), a way that seems perfectly legitimate. In my case, I am using these getters/setters to effectively create a pub/sub eventing model of changes on objects (liberal use of $watch was killing my application) - the console.log 'We just set test to ', v is effectively a basic demo of that. Whilst in theory I could use my own, home-grown angular.copy, it would be nice to have the core-exposed function work.

Is the intention of this call, source.hasOwnProperty(key), to effectively test whether the property is enumerable or not?

@petebacondarwin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myitcv - can you create an issue with a unit test that demonstrates this. I think it is a valid problem that we need to fix.

@petebacondarwin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myitcv - hold on a minute - the test property is being attached to the prototype so it is not a property on the a object.

@petebacondarwin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myitcv - I am not convinced that this is an issue. If the property is defined on the prototype then it gets passed through to the copy, simply because the copied object uses the same prototype as the source.
See this coffeescript example

@myitcv
Copy link
Contributor

@myitcv myitcv commented on b59b04f Jul 1, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petebacondarwin - agreed, the getters/setters are defined on the prototype. Furthermore, in this case they are defined as enumerable hence we would expect to see them when iterating the properties of a. _test on the other hand is defined on the object a and, more importantly, is defined as non-enumerable. Hence

console.log '(k for k of a):', (k for k, v of a) # ["test"] - does not include _test

But because the getters/setters are not direct properties we will not see them via:

console.log "a.hasOwnProperty('test'):", a.hasOwnProperty('test') # false
console.log '(k for own k of a):', (k for own k, v of a) # [] - again, does not include _test

The getters/setters are, however, properties nonetheless. So what I am arguing for is that angular.copy should be defined to use the (k for k of a) version because this correctly iterates the enumerable properties of a, whether direct properties or getters setters. If people (and this could include Angular core) don't want properties copied, make them non-enumerable, whether getters/setters or direct properties: _test is an example of the latter.

With reference to your linked example, the problem with the copy as implemented is as follows:

b = makeCopy(a)
console.log 'b.test', b.test # undefined - this is wrong

Why is this? Well we didn't iterate over test because of the own restriction. Instead I propose the following workingMakeCopy:

class A
  constructor: ->
  Object.defineProperty @prototype, 'test', 
    get: -> @["_test"]
    set: (v) -> 
      Object.defineProperty @, '_test',
        writable: true
        enumerable: false
      @["_test"] = v
      console.log 'We just set test to', v
    enumerable: true

a = new A
a.test = 5 # We just set test to 5
console.log 'a.test:', a.test # 5
console.log 'a._test:', a._test # 5
console.log 'Object.keys(a):', Object.keys(a) # []
console.log '(k for k of a):', (k for k, v of a) # ["test"]
console.log '(k for own k of a):', (k for own k, v of a) # []
console.log "a.hasOwnProperty('_test'):", a.hasOwnProperty('_test') # true
console.log "a.hasOwnProperty('test'):", a.hasOwnProperty('test') # false

makeCopy = (source) ->
  destination = Object.create(Object.getPrototypeOf(source))
  for own key, value of source
    destination[key] = value
  destination

workingMakeCopy = (source) ->
  destination = Object.create(Object.getPrototypeOf(source))
  for key, value of source
    destination[key] = value
  destination

b = makeCopy(a)
console.log 'b.test', b.test # undefined - this is wrong
console.log 'b._test', b._test # undefined - ... and by implication so is this
console.log 'b.test == a.test?', b.test == a.test # false - ... and hence this

c = workingMakeCopy(a) # We just set test to 5 - notice setter is triggered
console.log 'c.test', c.test # 5
console.log 'c._test', c._test # 5 - set via the prototype setter, not workingMakeCopy directly
console.log 'c.test == c.test?', c.test == a.test # true

Link to updated CoffeeScript example

@myitcv
Copy link
Contributor

@myitcv myitcv commented on b59b04f Jul 1, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petebacondarwin - just created #8032 with a test case (and proposed fix). Thoughts?

@gentooboontoo
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myitcv @petebacondarwin I have made a PR (#8034) which preserves own property descriptors (including getters/setters) and also keep own non-enumerable properties when copying object. @myitcv, could it solve your issue?

Please sign in to comment.