canReflect.assignDeep doesn't deep copy DefineMap

[green3g]

Code Sample:

import { Reflect, DefineMap } from "//unpkg.com/can@5/core.mjs";

const a = {};
const b = new DefineMap({
    prop: {
        test: 'hello'
    }
});

Reflect.assignDeep(a, b);

console.log(a.prop === b.prop); // true

https://codepen.io/anon/pen/rQOBPx?editors=0011

Objects nested inside a define map won't be deep copied, instead they will still reference the original object.

I may be wrong on this, but I would expect a to be a plain object with all of the properties from b copied over, not referenced.

[cherifGsoul]

The fix that I made seems correct, but it breaks can-map and can-define, any help how to solve this is much appreciated!

[chasenlehara]

Which can-define and can-map tests are failing with your change?

[cherifGsoul]

@chasenlehara those tests: https://github.com/canjs/can-map/blob/master/can-map_test.js#L108 https://github.com/canjs/can-map/blob/master/can-map_test.js#L585 https://github.com/canjs/can-define/blob/master/test/test-define-only.js#L398

[cherifGsoul]

I tried to find a solution by overwriting can.assignDeep in can-map but canReflect.assignDeep is called in the setup function to assign defaultValues to data object, the data has not assignDeep Symbol. Is this a breaking change or we need to add assignDeep Symbol to can-map data?

[justinbmeyer]

Can you describe what you did a bit more in depth. Ideally with snippets of the source you are taking about.

[cherifGsoul]

In my comment above I'm talking about this snippets in can-map:

var defaultValues = this._setupDefaults(obj);
var data = assign(canReflect.assignDeep({}, defaultValues), obj);

Fixing this issue will break can-map instantiation and tests like this will be broken. cc @justinbmeyer

[cherifGsoul]

I fixed the two broken tests for can-map caused by the fix of this issue: https://github.com/canjs/can-map/blob/master/can-map_test.js#L108 https://github.com/canjs/can-map/blob/master/can-map_test.js#L585

Here is the changes:

• in can-reflect I changed assignDeepMap to fix this issue, now with canReflect.assignDeep plain objects are copied not referenced

• To avoid the breaking change in can-map I added a assignDeepMap Symbol which has the behavior referencing Map objects in this PR

It remains the broken test in can-define.

[cherifGsoul]

The last changes/release in can-define make it works with my fix for this issue, now we just need to review/merge and release this can-map PR before releasing the fix of this issue.

[justinbmeyer]

I'm not totally sure this isn't expected behavior. I always expected assignDeep to do the smallest amount of assignment to get 2 objects shapes to match.

Though, jQuery and lodash do the same thing as what you expected: https://codepen.io/justinbmeyer/pen/YdZYvj

The odd thing here is types. You expect it to be a plain object, but I'm not sure that necessarily makes sense.

Why shouldn't a.prop and b.prop be a DefineMap?

[green3g]

I should have been more clear. I don't expect a.prop to be a plain object. I think it makes sense that a.prop is a DefineMap but a itself should be a plain object, since that's what is being passed.

Object.assign copies properties from one object to another, but it references nested properties. Therefore I would think assignDeep should copy properties (including deep ones) from one object to another, not reference them.

[justinbmeyer]

@roemhildtg Yeah to be clear, when assignDeep(destination, source) encounters a missing object on the destination, it should make a "clone" of the object in the source. It should then set that clone on destination.

I'm not sure we have a canReflect.clone(), but we could probably add one. clone() would probably need to rely on the .constructor property (it could check if obj.constructor.prototype === obj.__proto__).

We should check if lodash has a clone and how it works.

Also, lets say this encountered DOM ... could we teach it to use .cloneNode()?

[justinbmeyer]

import { Reflect, DefineMap } from "//unpkg.com/can@5/core.mjs";

const a = {};
const b = new DefineMap({
    prop: {
        test: 'hello'
    }
});

Reflect.assignDeep(a, b);

a //-> {prop: new DefineMap({test:"hello"}) }