Merge pull request #105 from gadget-inc/sc/skip-reaction-when-detached

spec/class-model-snapshotted-views.spec.ts

@@ -2,6 +2,7 @@ import { observable, runInAction } from "mobx";
 import { ClassModel, action, snapshottedView, getSnapshot, register, types, onPatch } from "../src";
 import { Apple } from "./fixtures/FruitAisle";
 import { create } from "./helpers";
+import { getParent } from "mobx-state-tree";
 
 @register
 class ViewExample extends ClassModel({ key: types.identifier, name: types.string }) {
@@ -228,4 +229,60 @@ describe("class model snapshotted views", () => {
       expect(getSnapshot(root)).toMatchSnapshot();
     });
   });
+
+  describe("parents", () => {
+    const onError = jest.fn();
+
+    @register
+    class Child extends ClassModel({}) {
+      @snapshottedView({ onError })
+      get parentsChildLength() {
+        const parent: Parent = getParent(this, 2);
+        return parent.children.size;
+      }
+    }
+
+    @register
+    class Parent extends ClassModel({ children: types.map(Child) }) {
+      @action
+      setChild(key: string, child: Child) {
+        this.children.set(key, child);
+      }
+
+      @action
+      createChild(key: string) {
+        this.children.set(key, {});
+        return this.children.get(key);
+      }
+
+      @action
+      removeChild(key: string) {
+        this.children.delete(key);
+      }
+    }
+
+    test("observable instances with views that access their parent will cause their view's reaction to error when created outside an action", () => {
+      Child.create();
+      expect(onError).toHaveBeenCalled();
+    });
+
+    test("observable instances with views that access their parent will not cause their view's reaction to error when created inside an action", () => {
+      runInAction(() => {
+        const child = Child.create();
+        const parent = Parent.create();
+        parent.setChild("foo", child);
+      });
+
+      expect(onError).not.toHaveBeenCalled();
+    });
+
+    test("observable instances don't run their view's reaction when they are destroyed", () => {
+      const parent = Parent.create();
+      parent.createChild("foo");
+      parent.removeChild("foo");
+
+      // would have errored if child.parentsChildLength was accessed after the child was removed
+      expect(onError).not.toHaveBeenCalled();
+    });
+  });
 });

src/class-model.ts

@@ -2,7 +2,7 @@ import "reflect-metadata";
 
 import memoize from "lodash.memoize";
 import type { Instance, IModelType as MSTIModelType, ModelActions } from "mobx-state-tree";
-import { isRoot, types as mstTypes } from "mobx-state-tree";
+import { types as mstTypes } from "mobx-state-tree";
 import { RegistrationError } from "./errors";
 import { InstantiatorBuilder } from "./fast-instantiator";
 import { FastGetBuilder } from "./fast-getter";
@@ -31,7 +31,7 @@ import type {
   TypesForModelPropsDeclaration,
 } from "./types";
 import { cyrb53 } from "./utils";
-import { comparer, reaction } from "mobx";
+import { comparer, reaction, type IReactionDisposer } from "mobx";
 import { getSnapshot } from "./snapshot";
 
 /** @internal */
@@ -48,6 +48,9 @@ export interface SnapshottedViewOptions<V, T extends IAnyClassModelType> {
 
   /** A function for converting the view value to a snapshot value */
   createSnapshot?: (value: V) => any;
+
+  /** A function that will be called when the view's reaction throws an error. */
+  onError?: (error: any) => void;
 }
 
 /** @internal */
@@ -295,28 +298,36 @@ export function register<Instance, Klass extends { new (...args: any[]): Instanc
       .props({ __snapshottedViewsEpoch: mstTypes.optional(mstTypes.number, 0) })
       .actions((self) => ({ __incrementSnapshottedViewsEpoch: () => self.__snapshottedViewsEpoch++ }))
       .actions((self) => {
-        const hook = isRoot(self) ? "afterCreate" : "afterAttach";
+        const reactions: IReactionDisposer[] = [];
+
         return {
-          [hook]() {
+          afterCreate() {
             for (const view of klass.snapshottedViews) {
-              reaction(
-                () => {
-                  const value = self[view.property];
-                  if (view.options.createSnapshot) {
-                    return view.options.createSnapshot(value);
-                  }
-                  if (Array.isArray(value)) {
-                    return value.map(getSnapshot);
-                  }
-                  return getSnapshot(value);
-                },
-                () => {
-                  self.__incrementSnapshottedViewsEpoch();
-                },
-                { equals: comparer.structural },
+              reactions.push(
+                reaction(
+                  () => {
+                    const value = self[view.property];
+                    if (view.options.createSnapshot) {
+                      return view.options.createSnapshot(value);
+                    }
+                    if (Array.isArray(value)) {
+                      return value.map(getSnapshot);
+                    }
+                    return getSnapshot(value);
+                  },
+                  () => {
+                    self.__incrementSnapshottedViewsEpoch();
+                  },
+                  { equals: comparer.structural, onError: view.options.onError },
+                ),
               );
             }
           },
+          beforeDestroy() {
+            for (const dispose of reactions) {
+              dispose();
+            }
+          },
         };
       });
   }