Mobx: Cyclic references broken in 3.*

[hazelsparrow]

Cyclic references appear to be broken in 3.0.0 and 3.1.11 (and quite possibly in every version in between, but I have not tested it on other 3.* versions).

The latest release where they still work is 2.7.0.

The code I'm running is

const {observable} = mobx;

const a = {};
const b = {a};
a.b = b;

const foo = observable(a);

console.log(foo);

I'm getting the following error

https://jsfiddle.net/Lgxdugo4/2/ <- works in 2.7

https://jsfiddle.net/qvegw4vk/2/ <- same code failing on 3.1.11

I was wondering if it's a 3.* bug or was the support for cyclic references dropped on purpose? I could look at the source code to try and identify the issue but since I'm not familiar with the codebase it may take a while. Maybe there is someone out there who can take a quick look?

Thanks!

[hazelsparrow]

This is the actual stack trace:

RangeError: Maximum call stack size exceeded
    at Object.getMessage (mobx/lib/utils/messages.js:42:20)
    at mobx/lib/api/extendobservable.js:29:67
    at Array.forEach (native)
    at extendObservableHelper (mobx/lib/api/extendobservable.js:28:16)
    at Object.extendObservable (mobx/lib/api/extendobservable.js:13:12)
    at Function.IObservableFactories.object (mobx/lib/api/observable.js:79:28)
    at deepEnhancer (mobx/lib/types/modifiers.js:25:40)
    at new ObservableValue (mobx/lib/types/observablevalue.js:27:23)
    at defineObservableProperty (mobx/lib/types/observableobject.js:94:45)
    at Object.defineObservablePropertyFromDescriptor (mobx/lib/types/observableobject.js:72:1

[hazelsparrow]

It looks like deepEnhancer should be aware of the objects already visited/encountered during the process of making a target object observable. In deepEnhancer, something like this maybe?

    // it is an observable already, done
    if (isObservable(v) **|| isAlreadyVisited(v)**)
        return v;

On another note, I'm still not sure why the first check does not work in the first place. In the example above, when you do observable(a), by the time deepEnhancer is called for the 2nd time with the v parameter equal to a (by traversing a->b->a), wouldn't a be already observable?

[urugator]

I'm still not sure why the first check does not work in the first place.

Maybe it's because the objects are first copied instead of made observable in place since 3.0 ... Therefore the b.a is not observable because it points to the original a and not to it's observable copy...

[mweststrate]

This change is intended indeed, see the 3.0 release notes: https://github.com/mobxjs/mobx/blob/master/CHANGELOG.md#300.

extendShallowObservable(obj, obj) can be used to update objects in place, however, you have to recursively apply that to get the old behavior. In general I recommend to create your data structures immediately as observables instead doing this after the fact. After the fact conversion to observables is typically done when receiving api responses and such, but those are always trees and never graphs.

[urugator]

Is it really necessary limitation? Can't Mobx create deep copy first and then perform patching inplace on the whole copy?

[mweststrate]

Since this is the first report of this missing feature, 6 months after the original release, this doesn't seem like a typical usage pattern. Adding this might significantly drop performance on large data sets, or require a Map/ Set polyfill to be present, which in itself would be a breaking change

[hazelsparrow]

Thanks for your responses, guys.

After the fact conversion to observables is typically done when receiving api responses and such, but those are always trees and never graphs.

In our case, we use JsonAPI which allows cyclic references. The API response itself is of course a tree, but we're not working with the data structures of the response directly; instead, we consume Javascript objects that a library processing that response generates, and those can certainly contain cyclic references. An example would be an Employee object that has a manager relation to another Employee.

Immediately creating observables, as opposed to doing that after the fact, is impossible without modifying the source code of the JsonAPI library we're using. But my main concern is that doing so would introduce a dependency of the JsonAPI library on MobX.

[mweststrate]

I think it is quite easy to mimic the old behavior, something like this should work (untested)

convertInPlace(obj) {
    if (!obj ||isObservable(obj))
        return obj
    if (Array.isArray(obj))
        return observable.array(obj.map(convertInPlace))
    if (typeof obj === "object") {
        extendObservable(obj, {}) // marks visited to avoid loops
        Object.keys(obj).forEach(key => {
            // convert each prop
            extendObservable(obj, { [key]: convertInPlace(obj[key]) })
        })
        return obj
    }
    return obj
}

[hazelsparrow]

@mweststrate thanks!

Should we add this as an opt-in feature? I'm willing to do a PR.

[mweststrate]

I think that would be better suited in mobx-utils. To here are so many different patterns already to create observables I rather restrain from adding even more options to the core library