New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

canReflect.assignDeep doesn't deep copy DefineMap #150

Open
roemhildtg opened this Issue Nov 6, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@roemhildtg
Copy link

roemhildtg commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Member

cherifGsoul commented Nov 29, 2018

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

This comment has been minimized.

Copy link
Member

chasenlehara commented Nov 29, 2018

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

@cherifGsoul

This comment has been minimized.

Copy link
Member

cherifGsoul commented Dec 5, 2018

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

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 6, 2018

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

@cherifGsoul

This comment has been minimized.

Copy link
Member

cherifGsoul commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Member

cherifGsoul commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Member

cherifGsoul commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 21, 2018

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?

@roemhildtg

This comment has been minimized.

Copy link

roemhildtg commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 21, 2018

@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

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 18, 2019

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"}) }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment