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

Incorrect behavior when trying to use customMerge with booleans #170

Closed
seva-luchianov opened this issue Oct 18, 2019 · 5 comments · Fixed by #172
Closed

Incorrect behavior when trying to use customMerge with booleans #170

seva-luchianov opened this issue Oct 18, 2019 · 5 comments · Fixed by #172

Comments

@seva-luchianov
Copy link

seva-luchianov commented Oct 18, 2019

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
Copy link
Owner

Looks like a bug caused by this line:

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

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
Copy link
Owner

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

@TehShrike
Copy link
Owner

A failing test would still be nice though.

@seva-luchianov
Copy link
Author

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

@TehShrike
Copy link
Owner

Yup. It's a pretty dumb bug.

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

Successfully merging a pull request may close this issue.

2 participants