Circular reference check false positive

[akinnee-gl]

Weird edge case, so feel free to say you won't fix it, but here's what I observed.

let object = {
    a: {},
    b: {}
};
let object2 = objectMerge( object, object.b );

This isn't a circular reference, but it passes your circular reference test.

[akinnee-gl]

Here's a workaround for this if anyone else runs into this issue and there is no fix implemented. This copies the object's properties onto a new object, which won't pass the circular reference test, but achieves the same result.

let object = {
    a: {},
    b: {}
};
let object2 = objectMerge(
    Object.assign( {}, object ),
    Object.assign( {}, object.b )
);

[matthewkastor]

function circularReferenceCheck(shadows) {
        // if any of the current objects to process exist in the queue
        // then throw an error.
        shadows.forEach(function (item) {
            if (item instanceof Object && visited.indexOf(item) > -1) {
                throw new Error('Circular reference error');
            }
        });

        // if none of the current objects were in the queue
        // then add references to the queue.
        visited = visited.concat(shadows);
}

In the first example you gave, I think the error might be that the merge sees property b in both items to be merged. You're effectively attempting to clone b into the given object which would take {a:{},b:{}} and merge b:{} onto it, effectively creating b:{a:{},b:{}} if it were successful BUT, this creates a circular reference during the process because the object is being traversed as it is being built from the clones. Seeing that object b:{} has already been processed as the new root, it's most likely blowing up when it tries to add the root object as a child of itself. In order to keep structural integrity of the tree, clones of b:{} need to maintain their identity, so that if you change the cloned b:{} in the merged object then it will change everywhere that you'd expect it to. . . That was my intention anyway... as in if you had two properties in your object graph that mapped to the same space in memory, changing one would change the other, and when you merge this object with another, you'd expect those two aliases for the underlying memory to retain their "by reference" nature, instead of becoming two copies of the data and breaking the relationship.

I believe the reason that the second example you give has worked, is that you have broken the relationship by assigning a new root to {a:{},b:{}} and b:{}. Grab the return result of the Object.assign calls and see if changing b on one object still changes b on the other object...

I don't have my debugger handy at the moment, this is just me thinking about the thing I wrote a few years ago and parsing it out in my head. I don't believe this is a bug, and it seems like fixing it would break a lot of code that already depends on this package, but I'm more than willing to admit when I'm wrong. If you would like to step through the code in a debugger and see what's happening I'd appreciate it, I don't have a lot of time to work on this really, I code all day at work and all night on metatrader projects. . . I mean my eyes are shot and my brain is just swimming right now.

[akinnee-gl]

That makes sense. I don't think it's something most people would run into, and if they do they can just create a new object with the properties of b first, like I did.