Skip to content
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

Merging objects discards target class information #208

Open
creativeux opened this issue Oct 25, 2020 · 6 comments · May be fixed by #209
Open

Merging objects discards target class information #208

creativeux opened this issue Oct 25, 2020 · 6 comments · May be fixed by #209

Comments

@creativeux
Copy link

creativeux commented Oct 25, 2020

For my use case, I am deserializing JSON to ES6 classes to assist in control flow and developer experience. I am using deepmerge to update objects stored in Easy Peasy, and I noticed that merging is causing the class information to be discarded, which is breaking my downstream use of instanceof for control flow purposes as well as likely confusing Easy Peasy / immer with new memory pointers for objects in state.

Specifically, this is where the issue lies in mergeObject:

	// This is causing type information to be discarded on merge
	var destination = {}

☝️ If I change this to assign the value of target to destination it works as I expect it to, but breaks other use cases.

I added the following unit test to show the behavior I expect, and it fails as I described above.

test('result should retain target type information', function(t) {
	var src = { key1: 'value1', key2: 'value2' }
	class CustomType {}
	var target = new CustomType()

	var res = merge(target, src)
	t.not(src instanceof CustomType)
	t.assert(target instanceof CustomType)
	t.assert(res instanceof CustomType) // <--- this is the failure point
	t.end()
})

Is this something that would be considered for support in the future?

@TehShrike
Copy link
Owner

So you want a deep assign merge, rather than a deep clone merge?

@creativeux
Copy link
Author

So you want a deep assign merge, rather than a deep clone merge?

Exactly right. 👍

@RebeccaStevens
Copy link

Wouldn't this me a dup of #186

@creativeux
Copy link
Author

creativeux commented Oct 29, 2020

Wouldn't this me a dup of #186

If I'm understanding it correctly, the clone option only takes effect on object values of properties being copied onto the target object. As I read deepmerge(), all object-based merges (non-array) pass through mergeObject() which immediately defines a new empty destination object.

Josh's comment on your linked issue summarizes it well, I think. The ask would be to allow destination to be initialized with the target value to retain the target's memory pointer and merge source into it rather than starting with an empty object and merging both objects into a new memory pointer.

@TehShrike
Copy link
Owner

Yeah, exactly what @creativeux said – the fix here would be to make the clone argument not be a lie when set to false, and to copy all values onto the target.

I would be open to a pull request that implemented that

@creativeux creativeux linked a pull request Oct 29, 2020 that will close this issue
@creativeux
Copy link
Author

@TehShrike - Ask and ye shall receive. Thank you for the quick discussion and feedback on the ask!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants