Properly destroy non-initialized nodes (fixes #1080) (#1082)

* test + fix

* better test description

Author: k-g-a

changelog.md

@@ -1,3 +1,7 @@
+# 3.8.1
+
+-   Fixed non-initialized nodes not being destroyed [#1080](https://github.com/mobxjs/mobx-state-tree/issues/1080) through [#1082](https://github.com/mobxjs/mobx-state-tree/pull/1082) by [@k-g-a](https://github.com/k-g-a)
+
 # 3.8.0
 
 -   Added castToSnapshot/castToReferenceSnapshot methods for TypeScript and fixed some TypeScript typings not being properly detected when using SnapshotIn types through [#1074](https://github.com/mobxjs/mobx-state-tree/pull/1074) by [@xaviergonz](https://github.com/xaviergonz)

packages/mobx-state-tree/__tests__/core/reference.test.ts

@@ -15,6 +15,7 @@ import {
     Instance,
     SnapshotOrInstance,
     isAlive,
+    destroy,
     castToReferenceSnapshot
 } from "../../src"
 
@@ -848,3 +849,82 @@ test("#1052 - Reference returns destroyed model after subtree replacing", () =>
         reactionDisposer2()
     }
 })
+
+it("#1080 - does not crash trying to resolve a reference to a destroyed+recreated model", () => {
+    const Branch = types.model("Branch", {
+        id: types.identifierNumber,
+        name: types.string
+    })
+
+    const User = types.model("User", {
+        id: types.identifierNumber,
+        email: types.maybeNull(types.string),
+        branches: types.maybeNull(types.array(Branch))
+    })
+
+    const BranchStore = types
+        .model("BranchStore", {
+            activeBranch: types.maybeNull(types.reference(Branch))
+        })
+        .actions(self => ({
+            setActiveBranch(branchId: any) {
+                self.activeBranch = branchId
+            }
+        }))
+
+    const RootStore = types
+        .model("RootStore", {
+            user: types.maybeNull(User),
+            branchStore: types.maybeNull(BranchStore)
+        })
+        .actions(self => ({
+            setUser(snapshot: typeof userSnapshot) {
+                self.user = cast(snapshot)
+            },
+            setBranchStore(snapshot: typeof branchStoreSnapshot) {
+                self.branchStore = cast(snapshot)
+            },
+            destroyUser() {
+                destroy(self.user!)
+            },
+            destroyBranchStore() {
+                destroy(self.branchStore!)
+            }
+        }))
+
+    const userSnapshot = {
+        id: 1,
+        email: "test@test.com",
+        branches: [
+            {
+                id: 1,
+                name: "Branch 1"
+            },
+            {
+                id: 2,
+                name: "Branch 2"
+            }
+        ]
+    }
+
+    const branchStoreSnapshot = {}
+    const rootStore = RootStore.create({ user: userSnapshot, branchStore: branchStoreSnapshot })
+
+    rootStore.branchStore!.setActiveBranch(1)
+    expect(rootStore.branchStore!.activeBranch).toEqual({
+        id: 1,
+        name: "Branch 1"
+    })
+
+    rootStore.destroyUser()
+    rootStore.destroyBranchStore()
+
+    rootStore.setUser(userSnapshot)
+    rootStore.setBranchStore(branchStoreSnapshot)
+
+    rootStore.branchStore!.setActiveBranch(2)
+    expect(rootStore.branchStore!.activeBranch).toEqual({
+        id: 2,
+        name: "Branch 2"
+    })
+})

packages/mobx-state-tree/src/core/node/object-node.ts

@@ -481,21 +481,17 @@ export class ObjectNode implements INode {
     @action
     public die() {
         if (this.state === NodeLifeCycle.DETACHING) return
-
         if (isStateTreeNode(this.storedValue)) {
-            // optimization: don't use walk, but getChildNodes for more efficiency
-            walk(this.storedValue, child => {
-                const node = getStateTreeNode(child)
-                if (node instanceof ObjectNode) node.aboutToDie()
-            })
-            walk(this.storedValue, child => {
-                const node = getStateTreeNode(child)
-                if (node instanceof ObjectNode) node.finalizeDeath()
-            })
+            this.aboutToDie()
+            this.finalizeDeath()
         }
     }
 
     public aboutToDie() {
+        this.getChildren().forEach(node => {
+            if (node instanceof ObjectNode) node.aboutToDie()
+        })
+
         // beforeDestroy should run before the disposers since else we could end up in a situation where
         // a disposer added with addDisposer at this stage (beforeDestroy) is actually never released
         this.fireHook("beforeDestroy")
@@ -508,6 +504,9 @@ export class ObjectNode implements INode {
 
     public finalizeDeath() {
         // invariant: not called directly but from "die"
+        this.getChildren().forEach(node => {
+            if (node instanceof ObjectNode) node.finalizeDeath()
+        })
         this.root.identifierCache!.notifyDied(this)
         addReadOnlyProp(this, "snapshot", this.snapshot) // kill the computed prop and just store the last snapshot