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

Copy prototype bug with enumerable properties #8032

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -808,14 +808,12 @@ function copy(source, destination, stackSource, stackDest) {
delete destination[key];
});
for ( var key in source) {
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;
result = copy(source[key], null, stackSource, stackDest);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not remove hasOwnProperty otherwise it will copy properties from prototype chain. I have replaced for...in and source.hasOwnProperty with Object.getOwnPropertyNames(source) to include non-enumerable properties in PR #8034 but I am not sure if this is what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gentooboontoo - no, we only want to consider enumerable properties. I think we're close to a solution with this

if (isObject(source[key])) {
stackSource.push(source[key]);
stackDest.push(result);
}
destination[key] = result;
}
setHashKey(destination,h);
}
Expand Down
93 changes: 93 additions & 0 deletions test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,99 @@ describe('angular', function() {
expect(copy(new Foo()) instanceof Foo).toBe(true);
});

it('should honour enumerable properties', function() {
var A, i, j, k;
i = 0;
j = 0;
k = 0;

A = (function() {
function A() {}

Object.defineProperty(A.prototype, 'test1', {
get: function() {
return this['_test1'];
},
set: function(v) {
Object.defineProperty(this, '_test1', {
writable: true,
enumerable: false
});
this['_test1'] = v;
return i++;
},
enumerable: true
});

Object.defineProperty(A.prototype, 'test2', {
get: function() {
return this['_test2'];
},
set: function(v) {
Object.defineProperty(this, '_test2', {
writable: true,
enumerable: true
});
this['_test2'] = v;
return j++;
},
enumerable: false
});

Object.defineProperty(A.prototype, 'test3', {
get: function() {
return this['_test3'];
},
set: function(v) {
Object.defineProperty(this, '_test3', {
writable: true,
enumerable: false
});
this['_test3'] = v;
return k++;
},
enumerable: false
});

return A;

})();

// [i,j,k] represents the number of times test1, test2 and
// test3 setters have been called

var a = new A();
expect([i,j,k]).toEqual([0,0,0]);
a.test1 = 1;
expect([i,j,k]).toEqual([1,0,0]);
a.test2 = 2;
expect([i,j,k]).toEqual([1,1,0]);
a.test3 = 3;
expect([i,j,k]).toEqual([1,1,1]);
expect(a.test1).toBe(1);
expect(a._test1).toBe(1);
expect(a.test2).toBe(2);
expect(a._test2).toBe(2);
expect(a.test3).toBe(3);
expect(a._test3).toBe(3);
var c = copy(a);

// we expect that the copy effectively called:
//
// c = Object.create(Object.getPrototypeOf(a))
// c.test1 = a.test1
// c._test2 = a._test2
// // nothing for test3

expect([i,j,k]).toEqual([2,1,1]);
expect(c.test1).toBe(1);
expect(c._test1).toBe(1);
expect(c.test2).toBe(2);
expect(c._test2).toBe(2);
expect(c.test3).toBeUndefined();
expect(c._test3).toBeUndefined();
});

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