trace(): log when computed becomes suspended (#2998)

* improve trace * changeset * changeset * adapt mobx-react test
mobxjs · Jun 24, 2021 · db21d85 · db21d85
1 parent 0f32387
commit db21d85
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 57 deletions.
diff --git a/.changeset/loud-rules-count.md b/.changeset/loud-rules-count.md
@@ -0,0 +1,6 @@
+---
+"mobx": patch
+---
+
+`trace()`: log when computed becomes suspended
+`requiresObserver` warns instead of throwing
diff --git a/packages/mobx-react/__tests__/issue806.test.tsx b/packages/mobx-react/__tests__/issue806.test.tsx
@@ -46,7 +46,7 @@ test("verify issue 806", () => {
         x.a.toString()
         expect(console.warn).toBeCalledTimes(1)
         expect(console.warn).toHaveBeenCalledWith(
-            "[mobx] Observable ObservableObject@1.a being read outside a reactive context"
+            "[mobx] Observable 'ObservableObject@1.a' being read outside a reactive context."
         )
     })
 })
diff --git a/packages/mobx/__tests__/v5/base/trace.ts b/packages/mobx/__tests__/v5/base/trace.ts
@@ -35,7 +35,7 @@ describe("trace", () => {
 
         x.fullname
         expectedLogCalls.push([
-            "[mobx.trace] 'x.fullname' is being read outside a reactive context. Doing a full recompute"
+            "[mobx.trace] Computed value 'x.fullname' is being read outside a reactive context. Doing a full recompute."
         ])
 
         const dispose = mobx.autorun(
@@ -61,6 +61,10 @@ describe("trace", () => {
 
         dispose()
 
+        expectedLogCalls.push([
+            "[mobx.trace] Computed value 'x.fullname' was suspended and it will recompute on the next access."
+        ])
+
         expect(expectedLogCalls).toEqual(consoleLogSpy.mock.calls)
     })
 
@@ -81,7 +85,7 @@ describe("trace", () => {
 
         x.fooIsGreaterThan5
         expectedLogCalls.push([
-            "[mobx.trace] 'x.fooIsGreaterThan5' is being read outside a reactive context. Doing a full recompute"
+            "[mobx.trace] Computed value 'x.fooIsGreaterThan5' is being read outside a reactive context. Doing a full recompute."
         ])
 
         const dispose = mobx.autorun(
@@ -113,6 +117,10 @@ describe("trace", () => {
 
         dispose()
 
+        expectedLogCalls.push([
+            "[mobx.trace] Computed value 'x.fooIsGreaterThan5' was suspended and it will recompute on the next access."
+        ])
+
         expect(expectedLogCalls).toEqual(consoleLogSpy.mock.calls)
     })
 

diff --git a/packages/mobx/__tests__/v5/base/typescript-decorators.ts b/packages/mobx/__tests__/v5/base/typescript-decorators.ts
@@ -1086,36 +1086,52 @@ test("1072 - @observable without initial value and observe before first access",
     observe(user, "loginCount", () => {})
 })
 
-test("unread computed reads should trow with requiresReaction enabled", () => {
-    class A {
-        @observable x = 0
+test("unobserved computed reads should warn with requiresReaction enabled", () => {
+    const consoleWarn = console.warn
+    const warnings: string[] = []
+    console.warn = function (...args) {
+        warnings.push(...args)
+    }
+    try {
+        const expectedWarnings: string[] = []
 
-        @computed({ requiresReaction: true })
-        get y() {
-            return this.x * 2
-        }
-        constructor() {
-            makeObservable(this)
+        class A {
+            @observable x = 0
+
+            @computed({ requiresReaction: true })
+            get y() {
+                return this.x * 2
+            }
+            constructor() {
+                makeObservable(this, undefined, { name: "a" })
+            }
         }
-    }
 
-    const a = new A()
-    expect(() => {
+        const a = new A()
+
         a.y
-    }).toThrow(/is read outside a reactive context/)
+        expectedWarnings.push(
+            `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.`
+        )
+
+        const d = mobx.reaction(
+            () => a.y,
+            () => {}
+        )
 
-    const d = mobx.reaction(
-        () => a.y,
-        () => {}
-    )
-    expect(() => {
         a.y
-    }).not.toThrow()
 
-    d()
-    expect(() => {
+        d()
+
         a.y
-    }).toThrow(/is read outside a reactive context/)
+        expectedWarnings.push(
+            `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.`
+        )
+
+        expect(warnings).toEqual(expectedWarnings)
+    } finally {
+        console.warn = consoleWarn
+    }
 })
 
 test("multiple inheritance should work", () => {

diff --git a/packages/mobx/__tests__/v5/base/typescript-tests.ts b/packages/mobx/__tests__/v5/base/typescript-tests.ts
@@ -1671,39 +1671,57 @@ test("issue #1122", done => {
     }, 100)
 })
 
-test("unread computed reads should trow with requiresReaction enabled", () => {
-    class A {
-        x = 0
+test("unobserved computed reads should warn with requiresReaction enabled", () => {
+    const consoleWarn = console.warn
+    const warnings: string[] = []
+    console.warn = function (...args) {
+        warnings.push(...args)
+    }
+    try {
+        const expectedWarnings: string[] = []
 
-        constructor() {
-            makeObservable(this, {
-                x: observable,
-                y: computed({ requiresReaction: true })
-            })
+        class A {
+            x = 0
+            get y() {
+                return this.x * 2
+            }
+            constructor() {
+                makeObservable(
+                    this,
+                    {
+                        x: observable,
+                        y: computed({ requiresReaction: true })
+                    },
+                    { name: "a" }
+                )
+            }
         }
 
-        get y() {
-            return this.x * 2
-        }
-    }
+        const a = new A()
 
-    const a = new A()
-    expect(() => {
         a.y
-    }).toThrow(/is read outside a reactive context/)
+        expectedWarnings.push(
+            `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.`
+        )
+
+        const d = mobx.reaction(
+            () => a.y,
+            () => {}
+        )
 
-    const d = mobx.reaction(
-        () => a.y,
-        () => {}
-    )
-    expect(() => {
         a.y
-    }).not.toThrow()
 
-    d()
-    expect(() => {
+        d()
+
         a.y
-    }).toThrow(/is read outside a reactive context/)
+        expectedWarnings.push(
+            `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.`
+        )
+
+        expect(warnings).toEqual(expectedWarnings)
+    } finally {
+        console.warn = consoleWarn
+    }
 })
 
 test("multiple inheritance should work", () => {

diff --git a/packages/mobx/src/core/computedvalue.ts b/packages/mobx/src/core/computedvalue.ts
@@ -255,6 +255,11 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
         if (!this.keepAlive_) {
             clearObserving(this)
             this.value_ = undefined // don't hold on to computed value!
+            if (this.isTracing_ !== TraceMode.NONE) {
+                console.log(
+                    `[mobx.trace] Computed value '${this.name_}' was suspended and it will recompute on the next access.`
+                )
+            }
         }
     }
 
@@ -283,17 +288,14 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 
     warnAboutUntrackedRead_() {
         if (!__DEV__) return
-        if (this.requiresReaction_ === true) {
-            die(`[mobx] Computed value ${this.name_} is read outside a reactive context`)
-        }
         if (this.isTracing_ !== TraceMode.NONE) {
             console.log(
-                `[mobx.trace] '${this.name_}' is being read outside a reactive context. Doing a full recompute`
+                `[mobx.trace] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.`
             )
         }
-        if (globalState.computedRequiresReaction) {
+        if (globalState.computedRequiresReaction || this.requiresReaction_) {
             console.warn(
-                `[mobx] Computed value ${this.name_} is being read outside a reactive context. Doing a full recompute`
+                `[mobx] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.`
             )
         }
     }

diff --git a/packages/mobx/src/core/derivation.ts b/packages/mobx/src/core/derivation.ts
@@ -149,7 +149,9 @@ export function checkIfStateModificationsAreAllowed(atom: IAtom) {
 
 export function checkIfStateReadsAreAllowed(observable: IObservable) {
     if (__DEV__ && !globalState.allowStateReads && globalState.observableRequiresReaction) {
-        console.warn(`[mobx] Observable ${observable.name_} being read outside a reactive context`)
+        console.warn(
+            `[mobx] Observable '${observable.name_}' being read outside a reactive context.`
+        )
     }
 }
 
@@ -195,7 +197,7 @@ function warnAboutDerivationWithoutDependencies(derivation: IDerivation) {
 
     if (globalState.reactionRequiresObservable || derivation.requiresObservable_) {
         console.warn(
-            `[mobx] Derivation ${derivation.name_} is created/updated without reading any observable value`
+            `[mobx] Derivation '${derivation.name_}' is created/updated without reading any observable value.`
         )
     }
 }