Fix #3492: throw warning when use class component and suspense

.changeset/chilly-nails-own.md

@@ -0,0 +1,5 @@
+---
+"mobx-react": patch
+---
+
+Fix #3492: throw warning when use class component and suspense

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

@@ -0,0 +1,216 @@
+import { act, cleanup, render } from "@testing-library/react"
+import * as mobx from "mobx"
+import * as React from "react"
+
+import { makeClassComponentObserver } from "../src/observerClass"
+import {
+    forceCleanupTimerToRunNowForTests,
+    resetCleanupScheduleForTests
+} from "../src/utils/reactionCleanupTracking"
+import {
+    CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS,
+    CLEANUP_TIMER_LOOP_MILLIS
+} from "../src/utils/reactionCleanupTrackingCommon"
+
+afterEach(cleanup)
+
+test("uncommitted components should not leak observations", async () => {
+    resetCleanupScheduleForTests()
+
+    // Unfortunately, Jest fake timers don't mock out Date.now, so we fake
+    // that out in parallel to Jest useFakeTimers
+    let fakeNow = Date.now()
+    jest.useFakeTimers()
+    jest.spyOn(Date, "now").mockImplementation(() => fakeNow)
+
+    const store = mobx.observable({ count1: 0, count2: 0 })
+
+    // Track whether counts are observed
+    let count1IsObserved = false
+    let count2IsObserved = false
+    mobx.onBecomeObserved(store, "count1", () => (count1IsObserved = true))
+    mobx.onBecomeUnobserved(store, "count1", () => (count1IsObserved = false))
+    mobx.onBecomeObserved(store, "count2", () => (count2IsObserved = true))
+    mobx.onBecomeUnobserved(store, "count2", () => (count2IsObserved = false))
+
+    class TestComponent1_ extends React.PureComponent {
+        render() {
+            return <div>{store.count1}</div>
+        }
+    }
+
+    const TestComponent1 = makeClassComponentObserver(TestComponent1_)
+
+    class TestComponent2_ extends React.PureComponent {
+        render() {
+            return <div>{store.count2}</div>
+        }
+    }
+
+    const TestComponent2 = makeClassComponentObserver(TestComponent2_)
+
+    // Render, then remove only #2
+    const rendering = render(
+        <React.StrictMode>
+            <TestComponent1 />
+            <TestComponent2 />
+        </React.StrictMode>
+    )
+    rendering.rerender(
+        <React.StrictMode>
+            <TestComponent1 />
+        </React.StrictMode>
+    )
+
+    // Allow any reaction-disposal cleanup timers to run
+    const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS)
+    fakeNow += skip
+    jest.advanceTimersByTime(skip)
+
+    // count1 should still be being observed by Component1,
+    // but count2 should have had its reaction cleaned up.
+    expect(count1IsObserved).toBeTruthy()
+    expect(count2IsObserved).toBeFalsy()
+})
+
+test("cleanup timer should not clean up recently-pended reactions", () => {
+    // If we're not careful with timings, it's possible to get the
+    // following scenario:
+    // 1. Component instance A is being created; it renders, we put its reaction R1 into the cleanup list
+    // 2. Strict/Concurrent mode causes that render to be thrown away
+    // 3. Component instance A is being created; it renders, we put its reaction R2 into the cleanup list
+    // 4. The MobX reaction timer from 5 seconds ago kicks in and cleans up all reactions from uncommitted
+    //    components, including R1 and R2
+    // 5. The commit phase runs for component A, but reaction R2 has already been disposed. Game over.
+
+    // This unit test attempts to replicate that scenario:
+    resetCleanupScheduleForTests()
+
+    // Unfortunately, Jest fake timers don't mock out Date.now, so we fake
+    // that out in parallel to Jest useFakeTimers
+    const fakeNow = Date.now()
+    jest.useFakeTimers()
+    jest.spyOn(Date, "now").mockImplementation(() => fakeNow)
+
+    const store = mobx.observable({ count: 0 })
+
+    // Track whether the count is observed
+    let countIsObserved = false
+    mobx.onBecomeObserved(store, "count", () => (countIsObserved = true))
+    mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false))
+
+    class TestComponent1_ extends React.PureComponent {
+        render() {
+            return <div>{store.count}</div>
+        }
+    }
+
+    const TestComponent1 = makeClassComponentObserver(TestComponent1_)
+
+    const rendering = render(
+        // We use StrictMode here, but it would be helpful to switch this to use real
+        // concurrent mode: we don't have a true async render right now so this test
+        // isn't as thorough as it could be.
+        <React.StrictMode>
+            <TestComponent1 />
+        </React.StrictMode>
+    )
+
+    // We need to trigger our cleanup timer to run. We can't do this simply
+    // by running all jest's faked timers as that would allow the scheduled
+    // `useEffect` calls to run, and we want to simulate our cleanup timer
+    // getting in between those stages.
+
+    // We force our cleanup loop to run even though enough time hasn't _really_
+    // elapsed.  In theory, it won't do anything because not enough time has
+    // elapsed since the reactions were queued, and so they won't be disposed.
+    forceCleanupTimerToRunNowForTests()
+
+    // Advance time enough to allow any timer-queued effects to run
+    jest.advanceTimersByTime(500)
+
+    // Now allow the useEffect calls to run to completion.
+    act(() => {
+        // no-op, but triggers effect flushing
+    })
+
+    // count should still be observed
+    expect(countIsObserved).toBeTruthy()
+})
+
+// TODO: MWE: disabled during React 18 migration, not sure how to express it icmw with testing-lib,
+// and using new React renderRoot will fail icmw JSDOM
+test.skip("component should recreate reaction if necessary", () => {
+    // There _may_ be very strange cases where the reaction gets tidied up
+    // but is actually still needed.  This _really_ shouldn't happen.
+    // e.g. if we're using Suspense and the component starts to render,
+    // but then gets paused for 60 seconds, and then comes back to life.
+    // With the implementation of React at the time of writing this, React
+    // will actually fully re-render that component (discarding previous
+    // hook slots) before going ahead with a commit, but it's unwise
+    // to depend on such an implementation detail.  So we must cope with
+    // the component having had its reaction tidied and still going on to
+    // be committed.  In that case we recreate the reaction and force
+    // an update.
+
+    // This unit test attempts to replicate that scenario:
+
+    resetCleanupScheduleForTests()
+
+    // Unfortunately, Jest fake timers don't mock out Date.now, so we fake
+    // that out in parallel to Jest useFakeTimers
+    let fakeNow = Date.now()
+    jest.useFakeTimers()
+    jest.spyOn(Date, "now").mockImplementation(() => fakeNow)
+
+    const store = mobx.observable({ count: 0 })
+
+    // Track whether the count is observed
+    let countIsObserved = false
+    mobx.onBecomeObserved(store, "count", () => (countIsObserved = true))
+    mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false))
+
+    class TestComponent1_ extends React.PureComponent {
+        render() {
+            return <div>{store.count}</div>
+        }
+    }
+
+    const TestComponent1 = makeClassComponentObserver(TestComponent1_)
+
+    const rendering = render(
+        <React.StrictMode>
+            <TestComponent1 />
+        </React.StrictMode>
+    )
+
+    // We need to trigger our cleanup timer to run. We don't want
+    // to allow Jest's effects to run, however: we want to simulate the
+    // case where the component is rendered, then the reaction gets cleaned up,
+    // and _then_ the component commits.
+
+    // Force everything to be disposed.
+    const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS)
+    fakeNow += skip
+    forceCleanupTimerToRunNowForTests()
+
+    // The reaction should have been cleaned up.
+    expect(countIsObserved).toBeFalsy()
+
+    // Whilst nobody's looking, change the observable value
+    store.count = 42
+
+    // Now allow the useEffect calls to run to completion,
+    // re-awakening the component.
+    jest.advanceTimersByTime(500)
+    act(() => {
+        // no-op, but triggers effect flushing
+    })
+
+    // count should be observed once more.
+    expect(countIsObserved).toBeTruthy()
+    // and the component should have rendered enough to
+    // show the latest value, which was set whilst it
+    // wasn't even looking.
+    expect(rendering.baseElement.textContent).toContain("42")
+})

packages/mobx-react/src/observerClass.ts

@@ -10,6 +10,11 @@ import {
 import { isUsingStaticRendering } from "mobx-react-lite"
 
 import { newSymbol, shallowEqual, setHiddenProp, patch } from "./utils/utils"
+import {
+    addReactionToTrack,
+    reactionTrack,
+    recordReactionAsCommitted
+} from "./utils/reactionCleanupTracking"
 
 const mobxAdminProperty = $mobx || "$mobx" // BC
 const mobxObserverProperty = newSymbol("isMobXReactObserver")
@@ -73,10 +78,26 @@ export function makeClassComponentObserver(
     }
     patch(target, "componentDidMount", function () {
         this[mobxIsUnmounted] = false
-        if (!this.render[mobxAdminProperty]) {
-            // Reaction is re-created automatically during render, but a component can re-mount and skip render #3395.
-            // To re-create the reaction and re-subscribe to relevant observables we have to force an update.
-            Component.prototype.forceUpdate.call(this)
+        recordReactionAsCommitted(this)
+        if (this[reactionTrack]) {
+            this[reactionTrack].mounted = true
+            if (!this.render[mobxAdminProperty] || this[reactionTrack].changedBeforeMount) {
+                // Reaction is re-created automatically during render, but a component can re-mount and skip render #3395.
+                // To re-create the reaction and re-subscribe to relevant observables we have to force an update.
+                Component.prototype.forceUpdate.call(this)
+                this[reactionTrack].changedBeforeMount = false
+            }
+        } else {
+            const initialName = getDisplayName(this)
+            this[reactionTrack] = {
+                reaction: new Reaction(`${initialName}.render()`, () => {
+                    // We've definitely already been mounted at this point
+                    Component.prototype.forceUpdate.call(this)
+                }),
+                mounted: true,
+                changedBeforeMount: false,
+                cleanAt: Infinity
+            }
         }
     })
     patch(target, "componentWillUnmount", function () {
@@ -143,7 +164,11 @@ function createReactiveRender(originalRender: any) {
                     try {
                         setHiddenProp(this, isForcingUpdateKey, true)
                         if (!this[skipRenderKey]) {
-                            Component.prototype.forceUpdate.call(this)
+                            if (this[reactionTrack].mounted) {
+                                Component.prototype.forceUpdate.call(this)
+                            } else {
+                                this[reactionTrack].changedBeforeMount = true
+                            }
                         }
                         hasError = false
                     } finally {
@@ -165,6 +190,11 @@ function createReactiveRender(originalRender: any) {
         isRenderingPending = false
         // Create reaction lazily to support re-mounting #3395
         const reaction = (reactiveRender[mobxAdminProperty] ??= createReaction())
+
+        if (!this[reactionTrack]) {
+            addReactionToTrack(this, reaction, {})
+        }
+
         let exception: unknown = undefined
         let rendering = undefined
         reaction.track(() => {

packages/mobx-react/src/utils/FinalizationRegistryWrapper.ts

@@ -0,0 +1,12 @@
+declare class FinalizationRegistryType<T> {
+    constructor(cleanup: (cleanupToken: T) => void)
+    register(object: object, cleanupToken: T, unregisterToken?: object): void
+    unregister(unregisterToken: object): void
+}
+
+declare const FinalizationRegistry: typeof FinalizationRegistryType | undefined
+
+const FinalizationRegistryLocal =
+    typeof FinalizationRegistry === "undefined" ? undefined : FinalizationRegistry
+
+export { FinalizationRegistryLocal as FinalizationRegistry }

packages/mobx-react/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts

@@ -0,0 +1,71 @@
+import { FinalizationRegistry as FinalizationRegistryMaybeUndefined } from "./FinalizationRegistryWrapper"
+import { Reaction } from "mobx"
+import {
+    ReactionCleanupTracking,
+    IReactionTracking,
+    createTrackingData,
+    Comp,
+    reactionTrack
+} from "./reactionCleanupTrackingCommon"
+
+/**
+ * FinalizationRegistry-based uncommitted reaction cleanup
+ */
+export function createReactionCleanupTrackingUsingFinalizationRegister(
+    FinalizationRegistry: NonNullable<typeof FinalizationRegistryMaybeUndefined>
+): ReactionCleanupTracking {
+    const cleanupTokenToReactionTrackingMap = new Map<number, IReactionTracking>()
+    let globalCleanupTokensCounter = 1
+
+    const registry = new FinalizationRegistry(function cleanupFunction(token: number) {
+        const trackedReaction = cleanupTokenToReactionTrackingMap.get(token)
+        if (trackedReaction) {
+            trackedReaction.reaction.dispose()
+            cleanupTokenToReactionTrackingMap.delete(token)
+        }
+    })
+
+    return {
+        /**
+         * TS type derivation cannot recognize symbol here. Manually set the type
+         */
+        addReactionToTrack(
+            reactionTrackingRef: Comp,
+            reaction: Reaction,
+            objectRetainedByReact: object
+        ) {
+            const token = globalCleanupTokensCounter++
+
+            registry.register(objectRetainedByReact, token, reactionTrackingRef)
+            reactionTrackingRef[reactionTrack] = createTrackingData(reaction)
+            ;(
+                reactionTrackingRef[reactionTrack] as IReactionTracking
+            ).finalizationRegistryCleanupToken = token
+            cleanupTokenToReactionTrackingMap.set(
+                token,
+                reactionTrackingRef[reactionTrack] as IReactionTracking
+            )
+
+            return reactionTrackingRef[reactionTrack] as IReactionTracking
+        },
+        recordReactionAsCommitted(reactionRef: Comp) {
+            registry.unregister(reactionRef)
+
+            if (
+                reactionRef[reactionTrack] &&
+                (reactionRef[reactionTrack] as IReactionTracking).finalizationRegistryCleanupToken
+            ) {
+                cleanupTokenToReactionTrackingMap.delete(
+                    (reactionRef[reactionTrack] as IReactionTracking)
+                        .finalizationRegistryCleanupToken as number
+                )
+            }
+        },
+        forceCleanupTimerToRunNowForTests() {
+            // When FinalizationRegistry in use, this this is no-op
+        },
+        resetCleanupScheduleForTests() {
+            // When FinalizationRegistry in use, this this is no-op
+        }
+    }
+}