Maximum call stack size exceeded

[ainglese]

when using observeAll, if the object is cyclic, an exception is thrown: Uncaught RangeError: Maximum call stack size exceeded

you can see a sample here: http://jsfiddle.net/k7t9yjrc/1/

is this by-design?

thanks!

[BorisMoore]

Hi ainlese,

Not exactly by design, but to be expected since there is currently no check against cyclic objects. I will look into whether support can be added for this scenario. I'll need to investigate further the possible fixes, before I can say whether this support will be able to be added... Added 'Planned for V1' but no guarantee yet :). Being optimistic!

[ainglese]

Ok, thank you. I've already redesigned my js to workaround this, so this is not a priority to me. I can suggest to update the docs because they states that "this means that no matter how complex the hierarchy of objects under the targeted object or array, and no matter how complex the structural changes made to that object hierarchy, the handler will continue to listen to any change on any object or array in the tree." witch is a bit misleading...

for the issue itself i can't think of many solution, if you cannot detect if change handler have already been attached the only way is to build a map of the object tree an pass it up to the recursive function that attaches the handlers. If you think this could be the right way i can try to fork and try a fix..

[BorisMoore]

Your objectMap solution is neat (https://github.com/BorisMoore/jsviews/pull/305) - but I have a couple of concerns: One is that [].indexOf() is not supported in IE8 - but that can be worked around by rewriting your test:

if ($.inArray(object, objMap) >= 0) {
    return;
}

The other - I think this leads to a memory leak - which is showing up when you run all tests: https://github.com/BorisMoore/jsviews/blob/master/test/UNIT-TESTS-ALL-JSVIEWS.html. They pass the first time, but the second time there are 30 that fail due to binding handlers not being removed.

I think it may be possible to detect the observeAll handler having been attached, without doing the objMap array, and I have started looking at a couple of such approaches that might work. If they do, then they would be preferable if it reduces additional API surface area, probably has better perf, and should avoid adding memory leaks.

Unfortunately I am currently travelling and not really able to give time to coding until I return in the second week of July. So I will probably not be able to follow through on this until then.

[BorisMoore]

I have a fix for this in the works. Details to follow.

[BorisMoore]

@ainglese: This was fixed in Commit 65. The fix is inspired by your proposed fix #305, but it turned out to be a fair bit more complex to achieve, since there are many scenarios where multiple objects can reference the same object. The fix required ref counting. It should now be good for almost all cyclic object graphs. One exception - that is not supported, is data with a cyclic graph which include duplicate ref to array properties, such as if you have: data.children = data.descendants = [].

See also the new unit tests added here: https://github.com/BorisMoore/jsviews/blob/master/test/unit-tests/tests-jsobservable.js#L832