TehShrike / deepmerge

A library for deep (recursive) merging of Javascript objects
MIT License
2.75k stars 216 forks source link

Merging objects discards target class information #208

Open creativeux opened 3 years ago

creativeux commented 3 years ago

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 commented 3 years ago

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

creativeux commented 3 years ago

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

Exactly right. 👍

RebeccaStevens commented 3 years ago

Wouldn't this me a dup of #186

creativeux commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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