Open seva-luchianov opened 4 years ago
This is expected behavior. The merge function is only called if isMergeableObject
returns true for the source value and the property exists on the target.
https://github.com/TehShrike/deepmerge/blob/d1ae60fb01f5c6b49a0e6b483bb2fb35c0995467/index.js#L66
I just published 4.2.2 which makes it so that isMergeableObject
is only called when there is a target value that could be merged: a34dd4d2
https://github.com/TehShrike/deepmerge/blob/e9c9fec24764837dd1ca178f86e8a5125cb93653/index.js#L66
I also flipped around the if/else code blocks to make the conditional a lot easier to read.
I see, thanks for clearing that up.
If I was to get the behavior that I want, I assume that I should traverse the target object after the merge is finished and just directly copy over keys that do not exist on it?
This feels like a nice feature to have in this library. Possibly a flag like mergeNewKeys
that defaults to false.
I assume that I should traverse the target object after the merge is finished and just directly copy over keys that do not exist on it?
That's what deepmerge does now: https://github.com/TehShrike/deepmerge/blob/e9c9fec24764837dd1ca178f86e8a5125cb93653/index.js#L66-L70
I'm not sure I understand. In my example, the bool
key of the target object was overwritten from true
to {}
when said key was not present in the object getting merged in. Why is this expected behavior?
Oh I'm sorry, I misread – you have the attribute on the target, but not on the source. So, in your case, it is actually going down the cloneUnlessOtherwiseSpecified
flow: https://github.com/TehShrike/deepmerge/blob/e9c9fec24764837dd1ca178f86e8a5125cb93653/index.js#L7-L11
And since isMergeableObject
is returning true, emptyTarget
is being called: https://github.com/TehShrike/deepmerge/blob/e9c9fec24764837dd1ca178f86e8a5125cb93653/index.js#L3-L5
Maybe that should be
getMergeFunction(key, options)(emptyTarget(value), value, options)
instead of
deepmerge(emptyTarget(value), value, options)
?
Right, so it's overwriting the target boolean key with {}
since it is not present on the source object. I do believe that is a bug
(Sorry for the early comment/accidental close there, I accidentally hit Cmd+Enter. I've edited that comment)
gotcha, i just reread that comment. I guess emptyTarget
should be dependent on the type stored in the key?
Maybe false
for booleans, and ""
for strings?
Out of the box, deepmerge only copies properties/elements of objects and arrays. And if an empty something isn't an array, it must be an object.
We could talk about making it possible to pass in a custom emptyTarget
option. Or, maybe your case could be worked around if the custom merge function was used inside the cloneUnlessOtherwiseSpecified
function.
Both seem like good options! Thanks for taking the time to consider this issue, I know I'm pushing the boundaries of what deepmerge is originally designed to offer.
Calling the customMerge
function inside cloneUnlessOtherwiseSpecified
would be the smallest stretch from the current functionality.
If you want to open a PR with a failing test, that would be appreciated.
Just made the PR
Hello, back with a similar bug to my previous issue. I updated to 4.2.1 and the boolean merge was indeed fixed, but it is broken when applied alongside other object types. For example:
will log:
seems like the boolean keys aren't getting logged in the custom merge function. This is the base case, though it also doesn't work in tandem with arrayMerge, which is my current use-case:
will log:
Thanks for the quick response to that previous issue! Using deepmerge 4.2.1 and is-mergeable-object 1.1.1