TehShrike / deepmerge

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

Incorrect behavior when trying to use customMerge with booleans #170

Closed seva-luchianov closed 5 years ago

seva-luchianov commented 5 years ago

I would like to utilize deepmerge to combine two objects that only have boolean keys in a way that keeps all booleans that were true in the destination object to remain true after the merge. Code is below:

const merge = require('deepmerge');

let output = booleanMerge({
    rootBool1: true,
    rootBool2: false,
    data: {
        nestedBool1: false,
        nestedBool2: true
    }
}, {
    rootBool1: false,
    rootBool2: true,
    data: {
        nestedBool1: true,
        nestedBool2: false
    }
});
console.log(output);

function booleanMerge(a, b) {
    return merge(a, b, {
        isMergeableObject: function(value) {
            return typeof value === "boolean" || require("is-mergeable-object")(value);
        },
        customMerge: function(key) {
            console.log("key:" + key);
            return function(a, b) {
                if (typeof a === "boolean" || typeof b === "boolean") {
                    console.log("merge bools: " + key + " " + a + " " + b);
                    return a || b;
                }
                console.log("not bools:");
                console.log(a);
                console.log(b);
                return booleanMerge(a, b);
            };
        }
    });
}

What I've found is that any boolean keys of the destination object that are false before the merge are never logged in the customMerge function, and end up as {} after the merge is complete. Here is the output after executing this code:

key:rootBool1
merge bools: rootBool1 true false
key:data
not bools:
{nestedBool1: false, nestedBool2: true}
{nestedBool1: true, nestedBool2: false}
key:nestedBool2
merge bools: nestedBool2 true false

// v The value of output v
{rootBool1: true, rootBool2: {}, data: {{nestedBool1: {}, nestedBool2: true}}}

Expected output is:

{rootBool1: true, rootBool2: true, data: {{nestedBool1: true, nestedBool2: true}}}

Using deepmerge 4.1.1 and is-mergeable-object 1.1.1

TehShrike commented 5 years ago

Looks like a bug caused by this line: https://github.com/TehShrike/deepmerge/blob/8209fc671a07b73ace6141fa743cae28f3e2481e/index.js#L47

It's checking if the value in the target is falsey, when it should be checking to see if the property doesn't exist in the target.

It would be great if someone could contribute a failing test.

The new test should probably use isMergeableObject: () => true and a customMerge function that asserts that it is called when asked to merge({ a: false }, { a: false })

TehShrike commented 5 years ago

Actually, I think this could be fixed by a tweak to #164...

TehShrike commented 5 years ago

A failing test would still be nice though.

seva-luchianov commented 5 years ago

I assume this will also fail for other falsey things like "" and 0?

TehShrike commented 5 years ago

Yup. It's a pretty dumb bug.