fix(mobx-react-lit): dispose reactions right after render in StrictMo…

…de and Suspense

onBecomeUnobserved never triggered with observer component mobxjs#3774

packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx

@@ -4,6 +4,7 @@ import * as mobx from "mobx"
 import * as React from "react"
 
 import { useObserver } from "../src/useObserver"
+import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession"
 
 afterEach(cleanup)
 
@@ -65,3 +66,53 @@ test(`observable changes before first commit are not lost`, async () => {
 
     expect(rendering.baseElement.textContent).toBe("changed")
 })
+
+test("should destroy reaction when Promise is thrown", async doneCallback => {
+    const o = mobx.observable({ x: 0, promise: null as Promise<void> | null })
+    const Cmp = () =>
+        useObserver(() => {
+            o.x as any // establish dependency
+            if (o.promise) {
+                throw o.promise
+            }
+            return o.x as any
+        })
+
+    const observed = jest.fn()
+    const unobserved = jest.fn()
+    mobx.onBecomeObserved(o, "x", observed)
+    mobx.onBecomeUnobserved(o, "x", unobserved)
+
+    const { container, unmount } = render(
+        <React.Suspense fallback={"loading..."}>
+            <Cmp />
+        </React.Suspense>
+    )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
+
+    expect(container).toHaveTextContent("0")
+    expect(observed).toBeCalledTimes(1)
+    expect(unobserved).toBeCalledTimes(0)
+    act(
+        mobx.action(() => {
+            o.promise = Promise.resolve()
+        })
+    )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
+    expect(container).toHaveTextContent("loading...")
+    expect(observed).toBeCalledTimes(1)
+    expect(unobserved).toBeCalledTimes(1)
+    act(
+        mobx.action(() => {
+            o.x++
+            o.promise = null
+        })
+    )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
+    await new Promise(resolve => setTimeout(resolve, 1))
+    expect(container).toHaveTextContent("1")
+    expect(observed).toBeCalledTimes(2)
+    expect(unobserved).toBeCalledTimes(1)
+
+    doneCallback()
+})

packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx

@@ -4,6 +4,7 @@ import * as React from "react"
 import { useObserver } from "../src/useObserver"
 import gc from "expose-gc/function"
 import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry"
+import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession"
 
 if (typeof globalThis.FinalizationRegistry !== "function") {
     throw new Error("This test must run with node >= 14")
@@ -35,11 +36,13 @@ test("uncommitted components should not leak observations", async () => {
             <TestComponent2 />
         </React.StrictMode>
     )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
     rendering.rerender(
         <React.StrictMode>
             <TestComponent1 />
         </React.StrictMode>
     )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
 
     // Allow gc to kick in in case to let finalization registry cleanup
     await new Promise(resolve => setTimeout(resolve, 100))

packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx

@@ -9,6 +9,7 @@ import {
 } from "../src/utils/UniversalFinalizationRegistry"
 import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry"
 import { TimerBasedFinalizationRegistry } from "../src/utils/UniversalFinalizationRegistry"
+import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession"
 
 expect(observerFinalizationRegistry).toBeInstanceOf(TimerBasedFinalizationRegistry)
 
@@ -45,11 +46,13 @@ test("uncommitted components should not leak observations", async () => {
             <TestComponent2 />
         </React.StrictMode>
     )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
     rendering.rerender(
         <React.StrictMode>
             <TestComponent1 />
         </React.StrictMode>
     )
+    requestAnimationFrameMock.triggerAllAnimationFrames()
 
     // Allow any reaction-disposal cleanup timers to run
     const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL)

packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts

@@ -0,0 +1,35 @@
+class RequestAnimationFrameMockSession {
+    handleCounter = 0
+    queue = new Map()
+    requestAnimationFrame(callback) {
+        const handle = this.handleCounter++
+        this.queue.set(handle, callback)
+        return handle
+    }
+    cancelAnimationFrame(handle) {
+        this.queue.delete(handle)
+    }
+    triggerNextAnimationFrame(time = performance.now()) {
+        const nextEntry = this.queue.entries().next().value
+        if (nextEntry === undefined) return
+
+        const [nextHandle, nextCallback] = nextEntry
+
+        nextCallback(time)
+        this.queue.delete(nextHandle)
+    }
+    triggerAllAnimationFrames(time = performance.now()) {
+        while (this.queue.size > 0) this.triggerNextAnimationFrame(time)
+    }
+    reset() {
+        this.queue.clear()
+        this.handleCounter = 0
+    }
+}
+
+export const requestAnimationFrameMock = new RequestAnimationFrameMockSession()
+
+window.requestAnimationFrame =
+    requestAnimationFrameMock.requestAnimationFrame.bind(requestAnimationFrameMock)
+window.cancelAnimationFrame =
+    requestAnimationFrameMock.cancelAnimationFrame.bind(requestAnimationFrameMock)

packages/mobx-react-lite/src/useObserver.ts

@@ -1,8 +1,7 @@
 import { Reaction } from "mobx"
-import React from "react"
+import React, { useLayoutEffect } from "react"
 import { printDebugValue } from "./utils/printDebugValue"
 import { isUsingStaticRendering } from "./staticRendering"
-import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry"
 import { useSyncExternalStore } from "use-sync-external-store/shim"
 
 // Required by SSR when hydrating #3669
@@ -34,11 +33,18 @@ function createReaction(adm: ObserverAdministration) {
     })
 }
 
+function disposeReaction(adm: ObserverAdministration) {
+    adm.onStoreChange = null
+    adm.reaction?.dispose()
+    adm.reaction = null
+}
+
 export function useObserver<T>(render: () => T, baseComponentName: string = "observed"): T {
     if (isUsingStaticRendering()) {
         return render()
     }
 
+    const animationRequestIDRef = React.useRef<number | null>(null)
     const admRef = React.useRef<ObserverAdministration | null>(null)
 
     if (!admRef.current) {
@@ -50,7 +56,6 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
             name: baseComponentName,
             subscribe(onStoreChange: () => void) {
                 // Do NOT access admRef here!
-                observerFinalizationRegistry.unregister(adm)
                 adm.onStoreChange = onStoreChange
                 if (!adm.reaction) {
                     // We've lost our reaction and therefore all subscriptions, occurs when:
@@ -66,9 +71,7 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
 
                 return () => {
                     // Do NOT access admRef here!
-                    adm.onStoreChange = null
-                    adm.reaction?.dispose()
-                    adm.reaction = null
+                    disposeReaction(adm)
                 }
             },
             getSnapshot() {
@@ -81,14 +84,11 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
     }
 
     const adm = admRef.current!
+    const firstRender = !adm.reaction
 
-    if (!adm.reaction) {
+    if (firstRender) {
         // First render or reaction was disposed by registry before subscribe
         createReaction(adm)
-        // StrictMode/ConcurrentMode/Suspense may mean that our component is
-        // rendered and abandoned multiple times, so we need to track leaked
-        // Reactions.
-        observerFinalizationRegistry.register(admRef, adm, adm)
     }
 
     React.useDebugValue(adm.reaction!, printDebugValue)
@@ -113,9 +113,28 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
         }
     })
 
+    if (animationRequestIDRef.current !== null) {
+        // cancel previous animation frame
+        cancelAnimationFrame(animationRequestIDRef.current)
+        animationRequestIDRef.current = null
+    }
+
+    // StrictMode/ConcurrentMode/Suspense may mean that our component is
+    // rendered and abandoned multiple times, so we need to dispose leaked
+    // Reactions.
+    animationRequestIDRef.current = requestAnimationFrame(() => {
+        disposeReaction(adm)
+    })
+
     if (exception) {
         throw exception // re-throw any exceptions caught during rendering
     }
 
+    const animationRequestID = animationRequestIDRef.current
+
+    useLayoutEffect(() => {
+        cancelAnimationFrame(animationRequestID)
+    })
+
     return renderResult
 }