fix: render reaction would dispose too early if observable data was c…

…hanged before first `useEffect` (#328)

* fix: reaction would dispose too early if observable data was cahnged before first render

* added changeset

.changeset/light-avocados-smile.md

@@ -0,0 +1,5 @@
+---
+"mobx-react-lite": patch
+---
+
+If observable data changed between mount and useEffect, the render reaction would incorrectly be disposed causing incorrect suspension of upstream computed values

src/useObserver.ts

@@ -42,10 +42,8 @@ export function useObserver<T>(fn: () => T, baseComponentName: string = "observe
                 forceUpdate()
             } else {
                 // We haven't yet reached useEffect(), so we'll need to trigger a re-render
-                // when (and if) useEffect() arrives.  The easiest way to do that is just to
-                // drop our current reaction and allow useEffect() to recreate it.
-                newReaction.dispose()
-                reactionTrackingRef.current = null
+                // when (and if) useEffect() arrives.
+                trackingData.changedBeforeMount = true
             }
         })
 
@@ -66,19 +64,25 @@ export function useObserver<T>(fn: () => T, baseComponentName: string = "observe
             // all we need to do is to record that it's now mounted,
             // to allow future observable changes to trigger re-renders
             reactionTrackingRef.current.mounted = true
+            // Got a change before first mount, force an update
+            if (reactionTrackingRef.current.changedBeforeMount) {
+                reactionTrackingRef.current.changedBeforeMount = false
+                forceUpdate()
+            }
         } else {
             // The reaction we set up in our render has been disposed.
-            // This is either due to bad timings of renderings, e.g. our
+            // This can be due to bad timings of renderings, e.g. our
             // component was paused for a _very_ long time, and our
-            // reaction got cleaned up, or we got a observable change
-            // between render and useEffect
+            // reaction got cleaned up
 
             // Re-create the reaction
             reactionTrackingRef.current = {
                 reaction: new Reaction(observerComponentNameFor(baseComponentName), () => {
                     // We've definitely already been mounted at this point
                     forceUpdate()
                 }),
+                mounted: true,
+                changedBeforeMount: false,
                 cleanAt: Infinity
             }
             forceUpdate()

src/utils/reactionCleanupTracking.ts

@@ -13,19 +13,21 @@ export interface IReactionTracking {
      * Whether the component has yet completed mounting (for us, whether
      * its useEffect has run)
      */
-    mounted?: boolean
+    mounted: boolean
 
     /**
      * Whether the observables that the component is tracking changed between
      * the first render and the first useEffect.
      */
-    changedBeforeMount?: boolean
+    changedBeforeMount: boolean
 }
 
 export function createTrackingData(reaction: Reaction) {
     const trackingData: IReactionTracking = {
-        cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS,
-        reaction
+        reaction,
+        mounted: false,
+        changedBeforeMount: false,
+        cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS
     }
     return trackingData
 }

test/observer.test.tsx

@@ -1,7 +1,7 @@
 import { act, cleanup, fireEvent, render } from "@testing-library/react"
 import mockConsole from "jest-mock-console"
 import * as mobx from "mobx"
-import * as React from "react"
+import React from "react"
 
 import { observer, useObserver, isObserverBatched, enableStaticRendering } from "../src"
 
@@ -867,3 +867,84 @@ it("should keep original props types", () => {
 //         /Cannot assign to read only property 'componentWillMount'/
 //     )
 // })
+
+it("dependencies should not become temporarily unobserved", async () => {
+    jest.spyOn(React, "useEffect")
+
+    let p: Promise<any>[] = []
+    const cleanups: any[] = []
+
+    async function runEffects() {
+        await Promise.all(p.splice(0))
+    }
+
+    // @ts-ignore
+    React.useEffect.mockImplementation(effect => {
+        console.warn("delaying useEffect call")
+        p.push(
+            new Promise(resolve => {
+                setTimeout(() => {
+                    act(() => {
+                        cleanups.push(effect())
+                    })
+                    resolve()
+                }, 10)
+            })
+        )
+    })
+
+    let computed = 0
+    let renders = 0
+
+    const store = mobx.makeAutoObservable({
+        x: 1,
+        get double() {
+            computed++
+            return this.x * 2
+        },
+        inc() {
+            this.x++
+        }
+    })
+
+    const doubleDisposed = jest.fn()
+    const reactionFired = jest.fn()
+
+    mobx.onBecomeUnobserved(store, "double", doubleDisposed)
+
+    const TestComponent = observer(() => {
+        renders++
+        return <div>{store.double}</div>
+    })
+
+    const r = render(<TestComponent />)
+
+    expect(computed).toBe(1)
+    expect(renders).toBe(1)
+    expect(doubleDisposed).toBeCalledTimes(0)
+
+    store.inc()
+    expect(computed).toBe(2) // change propagated
+    expect(renders).toBe(1) // but not yet rendered
+    expect(doubleDisposed).toBeCalledTimes(0) // if we dispose to early, this fails!
+
+    // Bug: change the state, before the useEffect fires, can cause the reaction to be disposed
+    mobx.reaction(() => store.x, reactionFired)
+    expect(reactionFired).toBeCalledTimes(0)
+    expect(computed).toBe(2) // Not 3!
+    expect(renders).toBe(1)
+    expect(doubleDisposed).toBeCalledTimes(0)
+
+    await runEffects()
+    expect(reactionFired).toBeCalledTimes(0)
+    expect(computed).toBe(2) // Not 3!
+    expect(renders).toBe(2)
+    expect(doubleDisposed).toBeCalledTimes(0)
+
+    r.unmount()
+    cleanups.filter(Boolean).forEach(f => f())
+    expect(reactionFired).toBeCalledTimes(0)
+    expect(computed).toBe(2)
+    expect(renders).toBe(2)
+    expect(doubleDisposed).toBeCalledTimes(1)
+})