deepObserve not working with ObservableSet

[gioragutt]

repro:

• repro.mjs

import {observable, runInAction} from 'mobx';
import {deepObserve} from 'mobx-utils';

const set = observable.set([1, 2, 3]);
const obj = observable({bla: true});

deepObserve(set, (change, path, root) => console.log(change, path, root));
deepObserve(obj, (change, path, root) => console.log(change, path, root));

console.log(set);

runInAction(() => {
  set.add('bla');
  obj.bla = false;
})

console output:

ObservableSet [Set] {
  name_: 'ObservableSet@1',
  data_: Set(3) { 1, 2, 3 },
  atom_: Atom {
    name_: 'ObservableSet@1',
    isPendingUnobservation_: false,
    isBeingObserved_: false,
    observers_: Set(0) {}, // <<<<<<<
    diffValue_: 0,
    lastAccessedBy_: 0,
    lowestObserverState_: 2,
    onBOL: undefined,
    onBUOL: undefined
  },
  changeListeners_: undefined,
  interceptors_: undefined,
  dehancer: undefined,
  enhancer_: [Function (anonymous)],
  [Symbol(mobx administration)]: {}
}

{
  type: 'update',
  observableKind: 'object',
  debugObjectName: 'ObservableObject@2',
  object: { bla: [Getter/Setter] },
  oldValue: true,
  name: 'bla',
  newValue: false
}  { bla: [Getter/Setter] }

Notice how in console.log(set) it has no observers.

[gioragutt]

Proposed change:

https://github.com/mobxjs/mobx-utils/blob/master/src/deepObserve.ts#L101

function observeRecursively(thing: any, parent: Entry | undefined, path: string) {
    if (isRecursivelyObservable(thing)) {
        const entry = entrySet.get(thing)
        if (entry) {
            if (entry.parent !== parent || entry.path !== path)
                // MWE: this constraint is artificial, and this tool could be made to work with cycles,
                // but it increases administration complexity, has tricky edge cases and the meaning of 'path'
                // would become less clear. So doesn't seem to be needed for now
                throw new Error(
                    `The same observable object cannot appear twice in the same tree,` +
                        ` trying to assign it to '${buildPath(parent)}/${path}',` +
                        ` but it already exists at '${buildPath(entry.parent)}/${entry.path}'`
                )
        } else {
            const entry = {
                parent,
                path,
                dispose: observe(thing, genericListener),
            }
            entrySet.set(thing, entry)
            entries(thing).forEach(([key, value]) => observeRecursively(value, entry, key))
        }
    // the else is added.
    } else if (isObservableSet(thing)) {
        const entry = {
            parent,
            path,
            dispose: observe(thing, genericListener),
        }
        entrySet.set(thing, entry)
    }
}

Tested and works on:

import {observable, runInAction} from 'mobx';
import {deepObserve} from 'mobx-utils';

const set = observable.set([1, 2, 3]);
deepObserve(set, (change, path, root) => console.log(change, path, root));
runInAction(() => set.add('bla'));

At first, I didn't add the isObservableSet(thing) check, and the following code failed:

import {observable, runInAction} from 'mobx';
import {deepObserve} from 'mobx-utils';

const obj = observable({
  bla: true,
  set: observable.set([1, 2, 3])
});

deepObserve(obj, (change, path, root) => console.log(change, path, root));
runInAction(() => obj.set.add('bla'));

With the following error:

/Users/gioraguttsait/Git/some-repo/node_modules/mobx/dist/mobx.cjs.development.js:91
    throw new Error("[MobX] " + e);
          ^

Error: [MobX] Cannot obtain administration from true
    at die (/Users/gioraguttsait/Git/some-repo/node_modules/mobx/dist/mobx.cjs.development.js:91:11)
    at getAdministration (/Users/gioraguttsait/Git/some-repo/node_modules/mobx/dist/mobx.cjs.development.js:5551:3)
    at observeObservable (/Users/gioraguttsait/Git/some-repo/node_modules/mobx/dist/mobx.cjs.development.js:3157:10)
    at Object.observe (/Users/gioraguttsait/Git/some-repo/node_modules/mobx/dist/mobx.cjs.development.js:3153:118)
    at observeRecursively (/Users/gioraguttsait/Git/some-repo/node_modules/mobx-utils/mobx-utils.umd.js:1195:35)
    at /Users/gioraguttsait/Git/some-repo/node_modules/mobx-utils/mobx-utils.umd.js:1188:32
    at Array.forEach (<anonymous>)
    at observeRecursively (/Users/gioraguttsait/Git/some-repo/node_modules/mobx-utils/mobx-utils.umd.js:1186:41)
    at deepObserve (/Users/gioraguttsait/Git/some-repo/node_modules/mobx-utils/mobx-utils.umd.js:1210:9)
    at file:///Users/gioraguttsait/Git/some-repo/mobile/test.mjs:9:1

[gioragutt]

Another proposed solution:

https://github.com/mobxjs/mobx-utils/blob/master/src/deepObserve.ts#L32

function isRecursivelyObservable(thing: any) {
    return isObservableObject(thing) || isObservableArray(thing) || isObservableMap(thing) || isObservableSet(thing);
}

After thinking about it, an ObservableSet can also be recursively observable (since it can hold observables). Therefore it shouldn't be treated differently from the other types.

Needless to say that code that failed before now works

[urugator]

Does it work for non-stringable entries? Set's values are also it's keys, so I wonder what happens with path...

[gioragutt]

Hmmm good point, I think that the entries in a Set are the items themselves as well, so that means that the path will probably be a toString of the observable item 🤭 I'll see how it looks