Skip to content

Commit 0e622f7

Browse files
jbedardlgalfaso
jbedard
authored and
lgalfaso
committed
fix(copy): do not copy the same object twice
1 parent 071be60 commit 0e622f7

2 files changed

Lines changed: 85 additions & 28 deletions

File tree

‎src/Angular.js‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -795,19 +795,33 @@ function copy(source, destination, stackSource, stackDest) {
795795

796796
if (!destination) {
797797
destination = source;
798-
if (source) {
798+
if (isObject(source)) {
799+
var index;
800+
if (stackSource && (index = stackSource.indexOf(source)) !== -1) {
801+
return stackDest[index];
802+
}
803+
804+
// TypedArray, Date and RegExp have specific copy functionality and must be
805+
// pushed onto the stack before returning.
806+
// Array and other objects create the base object and recurse to copy child
807+
// objects. The array/object will be pushed onto the stack when recursed.
799808
if (isArray(source)) {
800-
destination = copy(source, [], stackSource, stackDest);
809+
return copy(source, [], stackSource, stackDest);
801810
} else if (isTypedArray(source)) {
802811
destination = new source.constructor(source);
803812
} else if (isDate(source)) {
804813
destination = new Date(source.getTime());
805814
} else if (isRegExp(source)) {
806815
destination = new RegExp(source.source, source.toString().match(/[^\/]*$/)[0]);
807816
destination.lastIndex = source.lastIndex;
808-
} else if (isObject(source)) {
817+
} else {
809818
var emptyObject = Object.create(getPrototypeOf(source));
810-
destination = copy(source, emptyObject, stackSource, stackDest);
819+
return copy(source, emptyObject, stackSource, stackDest);
820+
}
821+
822+
if (stackDest) {
823+
stackSource.push(source);
824+
stackDest.push(destination);
811825
}
812826
}
813827
} else {
@@ -818,9 +832,6 @@ function copy(source, destination, stackSource, stackDest) {
818832
stackDest = stackDest || [];
819833

820834
if (isObject(source)) {
821-
var index = stackSource.indexOf(source);
822-
if (index !== -1) return stackDest[index];
823-
824835
stackSource.push(source);
825836
stackDest.push(destination);
826837
}
@@ -829,12 +840,7 @@ function copy(source, destination, stackSource, stackDest) {
829840
if (isArray(source)) {
830841
destination.length = 0;
831842
for (var i = 0; i < source.length; i++) {
832-
result = copy(source[i], null, stackSource, stackDest);
833-
if (isObject(source[i])) {
834-
stackSource.push(source[i]);
835-
stackDest.push(result);
836-
}
837-
destination.push(result);
843+
destination.push(copy(source[i], null, stackSource, stackDest));
838844
}
839845
} else {
840846
var h = destination.$$hashKey;
@@ -848,37 +854,27 @@ function copy(source, destination, stackSource, stackDest) {
848854
if (isBlankObject(source)) {
849855
// createMap() fast path --- Safe to avoid hasOwnProperty check because prototype chain is empty
850856
for (key in source) {
851-
putValue(key, source[key], destination, stackSource, stackDest);
857+
destination[key] = copy(source[key], null, stackSource, stackDest);
852858
}
853859
} else if (source && typeof source.hasOwnProperty === 'function') {
854860
// Slow path, which must rely on hasOwnProperty
855861
for (key in source) {
856862
if (source.hasOwnProperty(key)) {
857-
putValue(key, source[key], destination, stackSource, stackDest);
863+
destination[key] = copy(source[key], null, stackSource, stackDest);
858864
}
859865
}
860866
} else {
861867
// Slowest path --- hasOwnProperty can't be called as a method
862868
for (key in source) {
863869
if (hasOwnProperty.call(source, key)) {
864-
putValue(key, source[key], destination, stackSource, stackDest);
870+
destination[key] = copy(source[key], null, stackSource, stackDest);
865871
}
866872
}
867873
}
868874
setHashKey(destination,h);
869875
}
870876
}
871877
return destination;
872-
873-
function putValue(key, val, destination, stackSource, stackDest) {
874-
// No context allocation, trivial outer scope, easily inlined
875-
var result = copy(val, null, stackSource, stackDest);
876-
if (isObject(val)) {
877-
stackSource.push(val);
878-
stackDest.push(result);
879-
}
880-
destination[key] = result;
881-
}
882878
}
883879

884880
/**

‎test/AngularSpec.js‎

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ describe('angular', function() {
336336
expect(hashKey(dst)).not.toEqual(hashKey(src));
337337
});
338338

339-
it('should retain the previous $$hashKey', function() {
339+
it('should retain the previous $$hashKey when copying object with hashKey', function() {
340340
var src,dst,h;
341341
src = {};
342342
dst = {};
@@ -351,7 +351,21 @@ describe('angular', function() {
351351
expect(hashKey(dst)).toEqual(h);
352352
});
353353

354-
it('should handle circular references when circularRefs is turned on', function() {
354+
it('should retain the previous $$hashKey when copying non-object', function() {
355+
var dst = {};
356+
var h = hashKey(dst);
357+
358+
copy(null, dst);
359+
expect(hashKey(dst)).toEqual(h);
360+
361+
copy(42, dst);
362+
expect(hashKey(dst)).toEqual(h);
363+
364+
copy(new Date(), dst);
365+
expect(hashKey(dst)).toEqual(h);
366+
});
367+
368+
it('should handle circular references', function() {
355369
var a = {b: {a: null}, self: null, selfs: [null, null, [null]]};
356370
a.b.a = a;
357371
a.self = a;
@@ -362,13 +376,60 @@ describe('angular', function() {
362376

363377
expect(aCopy).not.toBe(a);
364378
expect(aCopy).toBe(aCopy.self);
379+
expect(aCopy).toBe(aCopy.selfs[2][0]);
365380
expect(aCopy.selfs[2]).not.toBe(a.selfs[2]);
381+
382+
var copyTo = [];
383+
aCopy = copy(a, copyTo);
384+
expect(aCopy).toBe(copyTo);
385+
expect(aCopy).not.toBe(a);
386+
expect(aCopy).toBe(aCopy.self);
387+
});
388+
389+
it('should handle objects with multiple references', function() {
390+
var b = {};
391+
var a = [b, -1, b];
392+
393+
var aCopy = copy(a);
394+
expect(aCopy[0]).not.toBe(a[0]);
395+
expect(aCopy[0]).toBe(aCopy[2]);
396+
397+
var copyTo = [];
398+
aCopy = copy(a, copyTo);
399+
expect(aCopy).toBe(copyTo);
400+
expect(aCopy[0]).not.toBe(a[0]);
401+
expect(aCopy[0]).toBe(aCopy[2]);
402+
});
403+
404+
it('should handle date/regex objects with multiple references', function() {
405+
var re = /foo/;
406+
var d = new Date();
407+
var o = {re: re, re2: re, d: d, d2: d};
408+
409+
var oCopy = copy(o);
410+
expect(oCopy.re).toBe(oCopy.re2);
411+
expect(oCopy.d).toBe(oCopy.d2);
412+
413+
oCopy = copy(o, {});
414+
expect(oCopy.re).toBe(oCopy.re2);
415+
expect(oCopy.d).toBe(oCopy.d2);
366416
});
367417

368418
it('should clear destination arrays correctly when source is non-array', function() {
369419
expect(copy(null, [1,2,3])).toEqual([]);
370420
expect(copy(undefined, [1,2,3])).toEqual([]);
371421
expect(copy({0: 1, 1: 2}, [1,2,3])).toEqual([1,2]);
422+
expect(copy(new Date(), [1,2,3])).toEqual([]);
423+
expect(copy(/a/, [1,2,3])).toEqual([]);
424+
expect(copy(true, [1,2,3])).toEqual([]);
425+
});
426+
427+
it('should clear destination objects correctly when source is non-array', function() {
428+
expect(copy(null, {0:1,1:2,2:3})).toEqual({});
429+
expect(copy(undefined, {0:1,1:2,2:3})).toEqual({});
430+
expect(copy(new Date(), {0:1,1:2,2:3})).toEqual({});
431+
expect(copy(/a/, {0:1,1:2,2:3})).toEqual({});
432+
expect(copy(true, {0:1,1:2,2:3})).toEqual({});
372433
});
373434

374435
it('should copy objects with no prototype parent', function() {

0 commit comments

Comments
 (0)