From 88b8e71710f11bffd77a0822986792036f5967cb Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Wed, 28 Nov 2018 00:16:43 +0100 Subject: [PATCH 1/9] deepAssign copy values instead of reference --- package.json | 1 + reflections/shape/shape-test.js | 13 +++++++++++++ reflections/shape/shape.js | 8 +++++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index da9c22a..d7c5886 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "can-symbol": "^1.3.0" }, "devDependencies": { + "can-define": "^2.7.1", "detect-cyclic-packages": "^1.1.0", "http-server": "^0.11.0", "jshint": "^2.9.1", diff --git a/reflections/shape/shape-test.js b/reflections/shape/shape-test.js index 7ead2ca..0924416 100644 --- a/reflections/shape/shape-test.js +++ b/reflections/shape/shape-test.js @@ -3,6 +3,7 @@ var canSymbol = require('can-symbol'); var shapeReflections = require("./shape"); var getSetReflections = require("../get-set/get-set"); var testHelpers = require('../../can-reflect-test_helpers'); +var DefineMap = require("can-define/map/map"); require("./schema/schema-test"); QUnit.module('can-reflect: shape reflections: own+enumerable'); @@ -711,6 +712,18 @@ QUnit.test("assignDeepList", function(){ ], "assigned right"); }); +QUnit.test("assignDeep copy #150", function() { + var obj = {}; + var objMap = new DefineMap({ + prop: { + foo: 'bar' + } + }); + shapeReflections.assignDeep(obj, objMap); + QUnit.notEqual(obj.prop, objMap.prop, "copy without referencing"); + +}) + /*QUnit.test("getAllEnumerableKeys", function(){ diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index 17235fc..6aedbb7 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -821,7 +821,13 @@ shapeReflections = { shapeReflections.eachKey(source, function(newVal, key){ if(!hasOwnKey(key)) { // set no matter what - getSetReflections.setKeyValue(target, key, newVal); + if (typeReflections.isMapLike(newVal)) { + // newVal needs to be serialized to make sure + // the value is copied not referenced + getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); + } else { + getSetReflections.setKeyValue(target, key, newVal); + } } else { var curVal = getKeyValue.call(target, key); From 603b86d1343114c6136576140e053638973ac164 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Wed, 28 Nov 2018 00:27:54 +0100 Subject: [PATCH 2/9] fix cyclic dependency --- package.json | 1 - reflections/shape/shape-test.js | 10 ++-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index d7c5886..da9c22a 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,6 @@ "can-symbol": "^1.3.0" }, "devDependencies": { - "can-define": "^2.7.1", "detect-cyclic-packages": "^1.1.0", "http-server": "^0.11.0", "jshint": "^2.9.1", diff --git a/reflections/shape/shape-test.js b/reflections/shape/shape-test.js index 0924416..24598ac 100644 --- a/reflections/shape/shape-test.js +++ b/reflections/shape/shape-test.js @@ -3,7 +3,6 @@ var canSymbol = require('can-symbol'); var shapeReflections = require("./shape"); var getSetReflections = require("../get-set/get-set"); var testHelpers = require('../../can-reflect-test_helpers'); -var DefineMap = require("can-define/map/map"); require("./schema/schema-test"); QUnit.module('can-reflect: shape reflections: own+enumerable'); @@ -714,15 +713,10 @@ QUnit.test("assignDeepList", function(){ QUnit.test("assignDeep copy #150", function() { var obj = {}; - var objMap = new DefineMap({ - prop: { - foo: 'bar' - } - }); + var objMap = {prop: { foo: 'bar' }}; shapeReflections.assignDeep(obj, objMap); QUnit.notEqual(obj.prop, objMap.prop, "copy without referencing"); - -}) +}); /*QUnit.test("getAllEnumerableKeys", function(){ From 7ec5792840b9320484de2469a5b63889a2c38962 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Thu, 6 Dec 2018 23:30:50 +0100 Subject: [PATCH 3/9] use assignDeepMap Symbol from target if available --- reflections/shape/shape.js | 49 ++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index 6aedbb7..602e267 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -813,34 +813,37 @@ shapeReflections = { return target; }, assignDeepMap: function(target, source) { - + var assignDeepMap = target[canSymbol.for("can.assignDeepMap")]; var hasOwnKey = fastHasOwnKey(target); var getKeyValue = target[getKeyValueSymbol] || shiftedGetKeyValue; var setKeyValue = target[setKeyValueSymbol] || shiftedSetKeyValue; - - shapeReflections.eachKey(source, function(newVal, key){ - if(!hasOwnKey(key)) { - // set no matter what - if (typeReflections.isMapLike(newVal)) { - // newVal needs to be serialized to make sure - // the value is copied not referenced - getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); - } else { - getSetReflections.setKeyValue(target, key, newVal); - } - } else { - var curVal = getKeyValue.call(target, key); - - // if either was primitive, no recursive update possible - if(newVal === curVal) { - // do nothing - } else if(typeReflections.isPrimitive(curVal) || typeReflections.isPrimitive(newVal) || shouldUpdateOrAssign(curVal) === false ) { - setKeyValue.call(target, key, newVal); + if (assignDeepMap) { + assignDeepMap.call(target, source); + } else { + shapeReflections.eachKey(source, function(newVal, key){ + if(!hasOwnKey(key)) { + // set no matter what + if (typeReflections.isMapLike(newVal)) { + // newVal needs to be serialized to make sure + // the value is copied not referenced + getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); + } else { + getSetReflections.setKeyValue(target, key, newVal); + } } else { - shapeReflections.assignDeep(curVal, newVal); + var curVal = getKeyValue.call(target, key); + + // if either was primitive, no recursive update possible + if(newVal === curVal) { + // do nothing + } else if(typeReflections.isPrimitive(curVal) || typeReflections.isPrimitive(newVal) || shouldUpdateOrAssign(curVal) === false ) { + setKeyValue.call(target, key, newVal); + } else { + shapeReflections.assignDeep(curVal, newVal); + } } - } - }, this); + }, this); + } return target; }, assignDeepList: function(target, source) { From bcb1bf85e0bc6bc39873530062c324f2f99e2352 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Tue, 11 Dec 2018 22:46:19 +0100 Subject: [PATCH 4/9] copy plain object only --- reflections/shape/shape.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index 602e267..9f2cda2 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -822,12 +822,12 @@ shapeReflections = { } else { shapeReflections.eachKey(source, function(newVal, key){ if(!hasOwnKey(key)) { - // set no matter what - if (typeReflections.isMapLike(newVal)) { - // newVal needs to be serialized to make sure - // the value is copied not referenced + // Plain objects needs to be serialized to make sure + // newVal is copied not referenced #150 + if (typeReflections.isMapLike(newVal) && !typeReflections.isObservableLike(newVal)) { getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); } else { + // set no matter what getSetReflections.setKeyValue(target, key, newVal); } } else { From 090e342ec6e08073f9ca96eca882e3b32925666e Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Tue, 11 Dec 2018 22:59:54 +0100 Subject: [PATCH 5/9] remove target assignDeepMap Symbol invocation --- reflections/shape/shape.js | 47 ++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index 9f2cda2..c074d2f 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -813,37 +813,34 @@ shapeReflections = { return target; }, assignDeepMap: function(target, source) { - var assignDeepMap = target[canSymbol.for("can.assignDeepMap")]; + var hasOwnKey = fastHasOwnKey(target); var getKeyValue = target[getKeyValueSymbol] || shiftedGetKeyValue; var setKeyValue = target[setKeyValueSymbol] || shiftedSetKeyValue; - if (assignDeepMap) { - assignDeepMap.call(target, source); - } else { - shapeReflections.eachKey(source, function(newVal, key){ - if(!hasOwnKey(key)) { - // Plain objects needs to be serialized to make sure - // newVal is copied not referenced #150 - if (typeReflections.isMapLike(newVal) && !typeReflections.isObservableLike(newVal)) { - getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); - } else { - // set no matter what - getSetReflections.setKeyValue(target, key, newVal); - } + + shapeReflections.eachKey(source, function(newVal, key){ + if(!hasOwnKey(key)) { + // Plain objects needs to be serialized to make sure + // newVal is copied not referenced #150 + if (typeReflections.isMapLike(newVal) && !typeReflections.isObservableLike(newVal)) { + getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); } else { - var curVal = getKeyValue.call(target, key); + // set no matter what + getSetReflections.setKeyValue(target, key, newVal); + } + } else { + var curVal = getKeyValue.call(target, key); - // if either was primitive, no recursive update possible - if(newVal === curVal) { - // do nothing - } else if(typeReflections.isPrimitive(curVal) || typeReflections.isPrimitive(newVal) || shouldUpdateOrAssign(curVal) === false ) { - setKeyValue.call(target, key, newVal); - } else { - shapeReflections.assignDeep(curVal, newVal); - } + // if either was primitive, no recursive update possible + if(newVal === curVal) { + // do nothing + } else if(typeReflections.isPrimitive(curVal) || typeReflections.isPrimitive(newVal) || shouldUpdateOrAssign(curVal) === false ) { + setKeyValue.call(target, key, newVal); + } else { + shapeReflections.assignDeep(curVal, newVal); } - }, this); - } + } + }, this); return target; }, assignDeepList: function(target, source) { From dcaebfb9343eae23d4570e702f3a17eb710358b5 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Fri, 14 Dec 2018 20:42:35 +0100 Subject: [PATCH 6/9] Fix and make the test reflect the issue --- reflections/shape/shape-test.js | 1 + reflections/shape/shape.js | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/reflections/shape/shape-test.js b/reflections/shape/shape-test.js index 24598ac..e30d428 100644 --- a/reflections/shape/shape-test.js +++ b/reflections/shape/shape-test.js @@ -714,6 +714,7 @@ QUnit.test("assignDeepList", function(){ QUnit.test("assignDeep copy #150", function() { var obj = {}; var objMap = {prop: { foo: 'bar' }}; + getSetReflections.setKeyValue(objMap.prop,canSymbol.for("can.onValue"), function(){}); shapeReflections.assignDeep(obj, objMap); QUnit.notEqual(obj.prop, objMap.prop, "copy without referencing"); }); diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index c074d2f..8639764 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -820,9 +820,10 @@ shapeReflections = { shapeReflections.eachKey(source, function(newVal, key){ if(!hasOwnKey(key)) { - // Plain objects needs to be serialized to make sure - // newVal is copied not referenced #150 - if (typeReflections.isMapLike(newVal) && !typeReflections.isObservableLike(newVal)) { + // Observable objects needs to be serialized + // when are assigned to plain objects + // newVal needs to be copied not referenced #150 + if (typeReflections.isPlainObject(target) && typeReflections.isObservableLike(newVal)) { getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); } else { // set no matter what From 4cfe859c13b6a017f9335a05c85a04d8fbac98b7 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Mon, 17 Dec 2018 21:44:15 +0100 Subject: [PATCH 7/9] add failing tests for depending repos scenarios --- reflections/shape/shape-test.js | 28 ++++++++++++++++++++++++++++ reflections/shape/shape.js | 18 +++++++++--------- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/reflections/shape/shape-test.js b/reflections/shape/shape-test.js index e30d428..9c8bafb 100644 --- a/reflections/shape/shape-test.js +++ b/reflections/shape/shape-test.js @@ -719,6 +719,34 @@ QUnit.test("assignDeep copy #150", function() { QUnit.notEqual(obj.prop, objMap.prop, "copy without referencing"); }); +QUnit.test("assign-deep with a constructor #150", function() { + function Example() {} + + Example.prototype = { + constructor: Example + }; + + var objMap = {}; + + shapeReflections.assignDeep({}, {nested: Example}); + ok(objMap.nested === Example); +}); + +QUnit.test("assign-deep with duplicate objects #150", function() { + var me = { + name: "Justin" + }; + + var references = { + husband: me, + friend: me + } + + var ref = {}; + shapeReflections.assignDeep(ref, references); + ok(ref.husband === ref.friend, "multiple properties point to the same thing") +}); + /*QUnit.test("getAllEnumerableKeys", function(){ diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index 8639764..373b3bb 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -817,18 +817,18 @@ shapeReflections = { var hasOwnKey = fastHasOwnKey(target); var getKeyValue = target[getKeyValueSymbol] || shiftedGetKeyValue; var setKeyValue = target[setKeyValueSymbol] || shiftedSetKeyValue; + var serialized = new Map(); + var serializedNewVal; - shapeReflections.eachKey(source, function(newVal, key){ + shapeReflections.eachKey(source, function(newVal, key){ if(!hasOwnKey(key)) { - // Observable objects needs to be serialized - // when are assigned to plain objects - // newVal needs to be copied not referenced #150 - if (typeReflections.isPlainObject(target) && typeReflections.isObservableLike(newVal)) { - getSetReflections.setKeyValue(target, key, shapeReflections.serialize(newVal)); - } else { - // set no matter what - getSetReflections.setKeyValue(target, key, newVal); + serializedNewVal = serialized.get(newVal); + if (!serializedNewVal) { + serializedNewVal = shapeReflections.serialize(newVal); + serialized.set(newVal, serializedNewVal); } + // set no matter what + getSetReflections.setKeyValue(target, key, serializedNewVal); } else { var curVal = getKeyValue.call(target, key); From 7392e553e63cc74f705f318f4a68913bba8331c2 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Mon, 17 Dec 2018 22:00:15 +0100 Subject: [PATCH 8/9] fix a test --- reflections/shape/shape-test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/reflections/shape/shape-test.js b/reflections/shape/shape-test.js index 9c8bafb..837ea91 100644 --- a/reflections/shape/shape-test.js +++ b/reflections/shape/shape-test.js @@ -720,7 +720,9 @@ QUnit.test("assignDeep copy #150", function() { }); QUnit.test("assign-deep with a constructor #150", function() { - function Example() {} + function Example() { + this.foo = "bar"; + } Example.prototype = { constructor: Example @@ -728,7 +730,7 @@ QUnit.test("assign-deep with a constructor #150", function() { var objMap = {}; - shapeReflections.assignDeep({}, {nested: Example}); + shapeReflections.assignDeep(objMap, {nested: Example}); ok(objMap.nested === Example); }); From 9c8353b5e7067cd948ddc7a5f803c6674cacb2d3 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Tue, 18 Dec 2018 22:13:43 +0100 Subject: [PATCH 9/9] make the tests pass --- reflections/shape/shape-test.js | 15 ++++++--------- reflections/shape/shape.js | 20 ++++++++++++-------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/reflections/shape/shape-test.js b/reflections/shape/shape-test.js index 837ea91..0ba54fe 100644 --- a/reflections/shape/shape-test.js +++ b/reflections/shape/shape-test.js @@ -720,13 +720,10 @@ QUnit.test("assignDeep copy #150", function() { }); QUnit.test("assign-deep with a constructor #150", function() { - function Example() { - this.foo = "bar"; - } - - Example.prototype = { - constructor: Example - }; + var Example = {}; + getSetReflections.setKeyValue(Example,canSymbol.for("can.new"), function(){ + return this; + }); var objMap = {}; @@ -742,11 +739,11 @@ QUnit.test("assign-deep with duplicate objects #150", function() { var references = { husband: me, friend: me - } + }; var ref = {}; shapeReflections.assignDeep(ref, references); - ok(ref.husband === ref.friend, "multiple properties point to the same thing") + ok(ref.husband === ref.friend, "multiple properties point to the same thing"); }); diff --git a/reflections/shape/shape.js b/reflections/shape/shape.js index 373b3bb..88ac4af 100644 --- a/reflections/shape/shape.js +++ b/reflections/shape/shape.js @@ -818,17 +818,21 @@ shapeReflections = { var getKeyValue = target[getKeyValueSymbol] || shiftedGetKeyValue; var setKeyValue = target[setKeyValueSymbol] || shiftedSetKeyValue; var serialized = new Map(); - var serializedNewVal; + var serializedNewVal; - shapeReflections.eachKey(source, function(newVal, key){ + shapeReflections.eachKey(source, function(newVal, key){ if(!hasOwnKey(key)) { - serializedNewVal = serialized.get(newVal); - if (!serializedNewVal) { - serializedNewVal = shapeReflections.serialize(newVal); - serialized.set(newVal, serializedNewVal); + if (newVal && (typeReflections.isConstructorLike(newVal) || typeReflections.isPrimitive(newVal) || typeReflections.isPlainObject(newVal) === false)) { + // set no matter what + getSetReflections.setKeyValue(target, key, newVal); + } else { + serializedNewVal = serialized.get(newVal); + if (!serializedNewVal) { + serializedNewVal = shapeReflections.serialize(newVal); + serialized.set(newVal, serializedNewVal); + } + setKeyValue.call(target, key, serializedNewVal); } - // set no matter what - getSetReflections.setKeyValue(target, key, serializedNewVal); } else { var curVal = getKeyValue.call(target, key);