Circular reference check

[matthewkastor]

Add a check for circular references to avoid infinite recursion.

[nullivex]

Check here https://github.com/snailjs/object-manage/blob/master/lib/ObjectManage.js#L141 for how I went about checking for circular references in objects.

[matthewkastor]

The code you pointed to looks like it's only checking for the depth of an object. A circular reference check would involve holding a reference to each property accessed and, for every new object perform a check on whether there's already a reference to it. For this sort of thing I usually use an array. When I first access an object (property) I'll do an indexOf(obj) on the array to see if I've already processed the object. If the check returns an index then I know I've encountered a circular reference.

The trouble I'm seeing with implementing a circular reference check here is offering the ability to switch it on and off. The way I process arguments would mean that adding any parameters to the function signature would be a breaking change. Right now, I can't think of any reason why someone would want to switch off a circular reference check so it's probably safe for me to just build this in without the option to disable it. Of course it's just after 10 at night so I can't say it's a good time of day to trust my decision making. Hmmm, I'd have to make sure I recreate the circular reference as well. That wouldn't be hard.

I should work on this in the morning so I don't miss something obvious. If you have ideas tonight send a pull request. For sure I want to add this in.

[nullivex]

Yes this is true, I was still letting the merge occur but throwing that warning as more of a convenience for debugging if a Maximum call stack size exceeded does get thrown, at least there would be a place to look.

And since a circular reference is guaranteed to fail merging I see no reason why it should be switchable. I would be interesting in seeing an instance where the merge would succeed.

I believe the depth parameter would pose a similar problem with the way arguments are parsed however it should be easy enough to check if the last argument is a number and take that as the depth parameter.

If the depth parameter were to be omitted it wouldn't be bad to throw some kind of warning similar to what I do if there is a horrendously deep object about to be merged. I think I was about to generate a call stack fail at like 500 levels of depth so it is possible without a circular reference (though unlikely).

[matthewkastor]

What you're saying makes sense. The only way I could see circular references ending in a successful merge would be in odd cases where property accesses augment the object, somehow eventually breaking the circular reference. Still, the clone wouldn't have the same structure as the original because it would contain clones of the nodes in whatever state they were in at the time they were visited. lolz, epiphany.

This definitely goes in without a way to disable it. I'm not masochistic enough to try recreating circular references and accounting for metaprogramming that augments objects on property access.

[matthewkastor]

I had a dream that I was working on this but it looks like I was doing something more like sleepwalking. XD The new version is 2.3.0, it's posted to npm. There are a few tests but nothing too robust: https://github.com/matthewkastor/object-merge/blob/master/browser/tests/object-merge.test.js#L149 Everything from line 149 to the end of that file (currently) covers the circular reference tests. Try it out a bit and see if it does what you expect.

[nullivex]

Haha, I had a dream about object-merging too. I worked too late I think.

Anyways, I grabbed the new version and gave it a whirl and its working great.

[nullivex]

Thanks man. I adjusted a few things and merged my branch back into mainline everything seems to be happy :)

[matthewkastor]

Awesome. :D I'll close this bug then.