onBecomeUnobserved is never called

[vitaliyslion]

Bug report

• [x] I've checked documentation and searched for existing issues and discussions
• [x] I've made sure my project is based on the latest MST version
• [x] Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

Codesandbox

Describe the expected behavior

When autorun is disposed, an observable should become unobserved and "onBecomeUnobserved" should be called

Describe the observed behavior

"onBecomeUnobserved" is never called

Hi. Our team tries to migrate a project to MST, and we have a lot of tests that we run for our models. While examining the tests I've found that some of them are failing because onBecomeUnobserved is never called. The test looks exactly the same as in the reproduction codesandbox. I noticed that this is also happening in the app, so the test itself seems to be fine. While using plain Mobx the same code worked well (I've actually found almost the same code in mobx's tests and took inspiration from there while writing mine).

[vitaliyslion]

Also, I used this issue as an inspiration for how onBecomeObserved and onBecomeUnobserved are used: https://github.com/mobxjs/mobx-state-tree/issues/987

[thegedge]

If I had to take a guess at why this is: there's a reaction against the snapshot of a tree node, so every property is always observed. If that's the case, I think it would be incredibly difficult for us to make this work without a fairly major refactor :'(

[coolsoftwaretyler]

Thanks for the report @vitaliyslion and for your initial thoughts @thegedge - I'll still take a look this weekend or next week and see. I'd love to understand the issue, myself.

[coolsoftwaretyler]

I think @thegedge is correct that this has to do with snapshot reactions. It looks like if a node isRoot, we add a reaction in the object-node code here: https://github.com/mobxjs/mobx-state-tree/blob/f87f96d5e2056ed2b8ce33487366de229c42d7c4/src/core/node/object-node.ts#L238

However, we should be running the disposer before the node dies. I think by disposing of the unobserved listener in beforeDestroy, the becomeUnobserved listener is missing the message because it's been disposed before MST had a chance to stop observing the snapshot.

See this modified code sandbox: https://codesandbox.io/p/sandbox/mst-onbecomeunobserved-bug-forked-pgzmv2?file=%2Fsrc%2Findex.mjs%3A48%2C1-49%2C1

import {
  getObserverTree,
  onBecomeObserved,
  onBecomeUnobserved,
  computed,
  autorun,
} from "mobx";
import { destroy, types } from "mobx-state-tree";

const log = (message) => {
  const element = document.createElement("div");

  element.innerHTML = message;

  document.body.append(message);
};

const model = types
  .model({ collection$: types.array(types.frozen()) })
  .actions((self) => {
    let becomeObsDisposer;
    let becomeUnobsDisposer;

    return {
      afterCreate() {
        becomeObsDisposer = onBecomeObserved(self, "collection$", () => {
          log("observed");
        });

        becomeUnobsDisposer = onBecomeUnobserved(self, "collection$", () => {
          log("unobserved");
        });
      },
      afterDestroy() {
        becomeObsDisposer();
        becomeUnobsDisposer();
      },
    };
  });

const instance = model.create({ collection$: [] });

const disposeAutorun = autorun(() => instance.collection$);

disposeAutorun();
destroy(instance);

console.log(getObserverTree(instance, "collection$"));

I made three changes here:

1. I removed your computed call. That was also observing the property, so unobserved wouldn't fire.
2. I changed beforeDestroy to afterDestroy and it seems to be working.
3. I started destroying the node to dispose of the snapshot reaction. This may not be practical for you.

Other things you could consider:

1. If you can attach these things in such a way that isRoot evaluated to false, we won't set up the reaction on the snapshot (I think).
2. If possible, instead of using standalone computed, use MobX-State-Tree views, which are computed values but should get destroyed correctly when the node is destroyed, and I think this will end up working as you want.
3. If you want to keep the standalone computed, find a way to dispose of it? I don't see anything in MobX about disposing standalone computed values. Maybe passing keepAlive: false would help? In my experimentation it didn't make a difference, but this seems like an angle to explore. https://mobx.js.org/computeds.html#keepalive
4. I used getObserverTree to get a list of the observers when I was troubleshooting. Thought that might be helpful in your own exploration.

Like @thegedge said, I don't think we can change our snapshot reaction behavior without a major change to the library, and it might even be fully breaking. Since afterDestroy works here, I'm inclined to call this not a bug.

@vitaliyslion - how do you feel about that? If you disagree, I'd be happy to keep talking about it until we get to a satisfactory resolution. I'll leave this open and if we don't hear from you in a week or so, I'll close it out (we can always reopen).

[coolsoftwaretyler]

Ah, this is trickier than I thought, even with child models. onSnapshot will also register a reaction:

https://github.com/mobxjs/mobx-state-tree/blob/f87f96d5e2056ed2b8ce33487366de229c42d7c4/src/core/node/object-node.ts#L576-L579

See here, we're still getting observed even with a parent/child tree: https://codesandbox.io/p/sandbox/mst-onbecomeunobserved-bug-forked-rztnfd?file=%2Fsrc%2Findex.mjs%3A33%2C9

But I can't see why the model's onSnapshot is getting called, unless autorun is doing that?

[coolsoftwaretyler]

Ok, two new hypotheses:

1. When the child is created, but before attaching, it's technically the root. So it gets a listener, but we never remove that listener when we attach. See https://github.com/mobxjs/mobx-state-tree/blob/f87f96d5e2056ed2b8ce33487366de229c42d7c4/src/core/node/object-node.ts#L238 and then https://github.com/mobxjs/mobx-state-tree/blob/f87f96d5e2056ed2b8ce33487366de229c42d7c4/src/core/node/object-node.ts#L286-L331
2. Even if we did dispose of it, the root-level observer would be observing all of the serializable properties, including child properties, so @thegedge's guess is correct, and I think I agree with the conclusion that there's not a clear path to changing this behavior.

I'm going to label this as can't fix for now, but I'd be open to more solutions, hopefully my investigation helps explain what we're seeing here.

[vitaliyslion]

3. I started destroying the node to dispose of the snapshot reaction. This may not be practical for you.

It's not practical, correct. For our case the collection$ is a persistent storage of e.g. transactions, users, comments, etc., basically a cache layer for our API responses. So this model will not be destroyed.

1. If you can attach these things in such a way that isRoot evaluated to false, we won't set up the reaction on the snapshot (I think).

I can see that reaction still exists even in that case: https://codesandbox.io/p/sandbox/mst-onbecomeunobserved-bug-forked-z3tcs8?file=%2Fsrc%2Findex.mjs%3A43%2C18&workspaceId=b264d118-589c-46f1-a79b-a83ce69dd0c6 But you've commented on that already, so this is expected. However, here it became even worse because even onBecomeObserved's callback was not called. Unless I did something wrong? I don't have much experience with MST, sorry.

Also, your codesanbox's links seems all to be private. Not a problem though, as you've printed the code here too.

[coolsoftwaretyler]

Hey @vitaliyslion - sorry about the privacy issues. I've made those sandboxes public now.

I'm not exactly sure why the first onBecomeObserved callback wasn't firing in your code, but I suspect it's another order-of-operations problem with afterCreate.

For disposers that come from callbacks like onBecomeObserved and onBecomeUnobserved, I would recommend holding those in volatile state. If you do that, it looks like the order-of-operations problems go away, so the onBecomeObserved callback fires in this code:

import { getObserverTree, onBecomeObserved, onBecomeUnobserved, autorun } from 'mobx'
import { destroy, types } from 'mobx-state-tree'

const CollectionStore = types
  .model({ collection$: types.array(types.frozen()) })
  .volatile((self) => ({
    becomeObsDisposer: onBecomeObserved(self, 'collection$', () => {
      console.log('become observed')
    }),
    becomeUnobsDisposer: onBecomeUnobserved(self, 'collection$', () => {
      console.log('become unobserved')
    }),
  }))
  .actions((self) => {
    return {
      beforeDestroy() {
        self.collection$.clear()
      },
      afterDestroy() {
        self.becomeObsDisposer()
        self.becomeUnobsDisposer()
      },
    }
  })

const root = types.model({ child: CollectionStore })

const rootInstance = root.create({
  child: { collection$: [{ name: '1' }, { name: '2' }] },
})

const disposeAutorun = autorun(() => {
  console.log('From autorun:', rootInstance.child.collection$)
})

console.log(getObserverTree(rootInstance.child, 'collection$'))

disposeAutorun()

console.log(getObserverTree(rootInstance.child, 'collection$'))

destroy(rootInstance)

The output for this code is:

become observed
From autorun: (2) [Object, Object]
{name: 'AnonymousModel.collection$', observers: Array(2)}
{name: 'AnonymousModel.collection$', observers: Array(1)}
become unobserved

See https://codesandbox.io/p/sandbox/mst-onbecomeunobserved-bug-forked-mt95z9?file=%2Fsrc%2Findex.mjs for it live.

Here are some notable properties of this code snippet:

1. I've made the CollectionStore instance a child of the rootInstance. I don't think this is necessary, but it seems likely you may end up with your collections nested like this, so I thought it was realistic. This code would work even if your CollectionStore instance stood in its own tree. See https://codesandbox.io/p/sandbox/mst-onbecomeunobserved-bug-forked-wk486c?file=%2Fsrc%2Findex.mjs%3A8%2C1.
2. I put the disposers in volatile state, so we can register them during instantiation rather than worrying about the callback order-of-operations and the parent.
3. I added a beforeDestroy hook to clear the collection$ array. I did this because the array itself will throw some warnings based on my destroy call at the end there. If you're not destroying these instances, this may not be necessary, but I wanted to keep the console output clean for example purposes.
4. If you look at the second getObserverTree, you'll still see the MST reaction observer hanging out there. That's probably intractable for us at this point.

I hope this helps to clarify. At this point, this is the best recommendation I have for you in terms of directly using the becomeObserved and becomeUnobserved utilities with MobX-State-Tree. We'll keep this issue around because I think it illustrates an important design challenge for us moving forward: how can we really truly expose MobX APIs when necessary to end users.

[vitaliyslion]

@coolsoftwaretyler thank you for your input. Regarding volatile state, my idea was to keep onBecomeObserved and onBecomeUnobserved disposers kind of private, this is why I hesitated using the volatile state. It makes it possible to call disposers before they actually should be called.

I guess at this point we will at first try to redesign our app to not use onBecomeObserved. I consider it unpredictable, and hard to debug. So anyway we were moving towards that solution, but at first I though that migration to MST would be a better idea and a higher priority.

[coolsoftwaretyler]

@vitaliyslion - looks like you can use a closure over those callbacks if you want the private access pattern, just avoid using the lifecycle hook for this.

I still typically prefer volatile, but I understand if that's not the design you want to use. Check it out:

import {
  getObserverTree,
  onBecomeObserved,
  onBecomeUnobserved,
  autorun,
} from "mobx";
import { destroy, types } from "mobx-state-tree";

const CollectionStore = types
  .model({ collection$: types.array(types.frozen()) })
  .actions((self) => {
    const becomeObsDisposer = onBecomeObserved(self, "collection$", () => {
      console.log("become observed");
    });
    const becomeUnobsDisposer = onBecomeUnobserved(self, "collection$", () => {
      console.log("become unobserved");
    });

    return {
      beforeDestroy() {
        self.collection$.clear();
      },
      afterDestroy() {
        becomeObsDisposer();
        becomeUnobsDisposer();
      },
    };
  });

const root = types.model({ child: CollectionStore });

const rootInstance = root.create({
  child: { collection$: [{ name: "1" }, { name: "2" }] },
});

const disposeAutorun = autorun(() => {
  console.log("From autorun:", rootInstance.child.collection$);
});

console.log(getObserverTree(rootInstance.child, "collection$"));

disposeAutorun();

console.log(getObserverTree(rootInstance.child, "collection$"));

destroy(rootInstance);

https://codesandbox.io/p/sandbox/mst-onbecomeunobserved-bug-forked-53wgd9?file=%2Fsrc%2Findex.mjs%3A47%2C1