customMerge should probably be called when cloning mergeable values

[seva-luchianov]

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:

function booleanMerge(a, b) {
    return merge(a, b, {
        isMergeableObject: function(value) {
            console.log("value:", value);
            return typeof value === "boolean" || isMergeableObject(value);
        },
        customMerge: function(key) {
            console.log("key:", key);
            return function(a, b) {
                console.log("merge:", a, b);
                if (typeof a === "boolean" && typeof b === "boolean") {
                    return a || b;
                }
                return booleanMerge(a, b, options);
            };
        }
    });
}
console.log("result:", booleanMerge({
    list: [1, 2, 3],
    bool: true
}, {
    list: [2, 3, 4]
}));

will log:

value: {list: [1, 2, 3], bool: true}
value: [1, 2, 3]
value: 1
value: 2
value: 3
value: true
value: {}
value: [2, 3, 4]
key: list
merge: [1, 2, 3] [2, 3, 4]
value: 1
value: 2
value: 3
value: 2
value: 3
value: 4
result: { list: (6) [1, 2, 3, 2, 3, 4], bool: {} }

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:

function booleanMerge(a, b) {
    return merge(a, b, {
        isMergeableObject: function(value) {
            console.log("value:", value);
            return typeof value === "boolean" || isMergeableObject(value);
        },
        customMerge: function(key) {
            console.log("key:", key);
            return function(a, b) {
                console.log("merge:", a, b);
                if (typeof a === "boolean" && typeof b === "boolean") {
                    return a || b;
                }
                return booleanMerge(a, b, options);
            };
        },
        arrayMerge: function(target, source) {
            const destination = target.slice();
            source.forEach(function(e) {
                if (!target.includes(e)) {
                    destination.push(e);
                }
            });
            return destination;
        }
    });
}
console.log("result:", booleanMerge({
    list: [1, 2, 3],
    bool: true
}, {
    list: [2, 3, 4]
}));

will log:

value: {list: [1, 2, 3], bool: true}
value: [1, 2, 3]
value: true
value: {}
value: [2, 3, 4]
key: list
merge: [1, 2, 3] [2, 3, 4]
result: { list: (6) [1, 2, 3, 4], bool: {} }

Thanks for the quick response to that previous issue!
Using deepmerge 4.2.1 and is-mergeable-object 1.1.1

[TehShrike]

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.

deepmerge/index.js

Line 66 in d1ae60f

if (!options.isMergeableObject(source[key]) || !propertyIsOnObject(target, key)) {

[TehShrike]

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: a34dd4d

deepmerge/index.js

Line 66 in e9c9fec

if (propertyIsOnObject(target, key) && options.isMergeableObject(source[key])) {

I also flipped around the if/else code blocks to make the conditional a lot easier to read.

[seva-luchianov]

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.

[TehShrike]

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:

deepmerge/index.js

Lines 66 to 70 in e9c9fec

	if (propertyIsOnObject(target, key) && options.isMergeableObject(source[key])) {
	destination[key] = getMergeFunction(key, options)(target[key], source[key], options)
	} else {
	destination[key] = cloneUnlessOtherwiseSpecified(source[key], options)
	}

[seva-luchianov]

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?

[TehShrike]

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:

deepmerge/index.js

Lines 7 to 11 in e9c9fec

	function cloneUnlessOtherwiseSpecified(value, options) {
	return (options.clone !== false && options.isMergeableObject(value))
	? deepmerge(emptyTarget(value), value, options)
	: value
	}

And since isMergeableObject is returning true, emptyTarget is being called:

deepmerge/index.js

Lines 3 to 5 in e9c9fec

	function emptyTarget(val) {
	return Array.isArray(val) ? [] : {}
	}

Maybe that should be

getMergeFunction(key, options)(emptyTarget(value), value, options)

instead of

deepmerge(emptyTarget(value), value, options)

?

[seva-luchianov]

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

[TehShrike]

(Sorry for the early comment/accidental close there, I accidentally hit Cmd+Enter. I've edited that comment)

[seva-luchianov]

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?

[TehShrike]

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.

[seva-luchianov]

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.

[TehShrike]

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.

[seva-luchianov]

Just made the PR