diff --git a/.changeset/chilly-nails-own.md b/.changeset/chilly-nails-own.md new file mode 100644 index 0000000000..0dd5266a30 --- /dev/null +++ b/.changeset/chilly-nails-own.md @@ -0,0 +1,5 @@ +--- +"mobx-react": patch +--- + +Fix #3492: throw warning when use class component and suspense diff --git a/packages/mobx-react/__tests__/strictAndConcurrentModeUsingTimers.test.tsx b/packages/mobx-react/__tests__/strictAndConcurrentModeUsingTimers.test.tsx new file mode 100644 index 0000000000..7fc8c43ae1 --- /dev/null +++ b/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
{store.count1}
+ } + } + + const TestComponent1 = makeClassComponentObserver(TestComponent1_) + + class TestComponent2_ extends React.PureComponent { + render() { + return
{store.count2}
+ } + } + + const TestComponent2 = makeClassComponentObserver(TestComponent2_) + + // Render, then remove only #2 + const rendering = render( + + + + + ) + rendering.rerender( + + + + ) + + // 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
{store.count}
+ } + } + + 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. + + + + ) + + // 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
{store.count}
+ } + } + + const TestComponent1 = makeClassComponentObserver(TestComponent1_) + + const rendering = render( + + + + ) + + // 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") +}) diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index 7a2c6be1a4..79c9da3e0c 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/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(() => { diff --git a/packages/mobx-react/src/utils/FinalizationRegistryWrapper.ts b/packages/mobx-react/src/utils/FinalizationRegistryWrapper.ts new file mode 100644 index 0000000000..d110063038 --- /dev/null +++ b/packages/mobx-react/src/utils/FinalizationRegistryWrapper.ts @@ -0,0 +1,12 @@ +declare class FinalizationRegistryType { + 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 } diff --git a/packages/mobx-react/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts b/packages/mobx-react/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts new file mode 100644 index 0000000000..c48dbe689f --- /dev/null +++ b/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 +): ReactionCleanupTracking { + const cleanupTokenToReactionTrackingMap = new Map() + 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 + } + } +} diff --git a/packages/mobx-react/src/utils/createTimerBasedReactionCleanupTracking.ts b/packages/mobx-react/src/utils/createTimerBasedReactionCleanupTracking.ts new file mode 100644 index 0000000000..50f8a683e3 --- /dev/null +++ b/packages/mobx-react/src/utils/createTimerBasedReactionCleanupTracking.ts @@ -0,0 +1,119 @@ +import { Reaction } from "mobx" +import { + ReactionCleanupTracking, + CLEANUP_TIMER_LOOP_MILLIS, + createTrackingData, + Comp, + reactionTrack +} from "./reactionCleanupTrackingCommon" + +/** + * timers, gc-style, uncommitted reaction cleanup + */ +export function createTimerBasedReactionCleanupTracking(): ReactionCleanupTracking { + /** + * Reactions created by components that have yet to be fully mounted. + */ + const uncommittedReactionRefs: Set = new Set() + + /** + * Latest 'uncommitted reactions' cleanup timer handle. + */ + let reactionCleanupHandle: ReturnType | undefined + + /* istanbul ignore next */ + /** + * Only to be used by test functions; do not export outside of mobx-react-lite + */ + function forceCleanupTimerToRunNowForTests() { + // This allows us to control the execution of the cleanup timer + // to force it to run at awkward times in unit tests. + if (reactionCleanupHandle) { + clearTimeout(reactionCleanupHandle) + cleanUncommittedReactions() + } + } + + /* istanbul ignore next */ + function resetCleanupScheduleForTests() { + if (uncommittedReactionRefs.size > 0) { + for (const ref of uncommittedReactionRefs) { + const tracking = ref[reactionTrack] + if (tracking) { + tracking.reaction.dispose() + ref[reactionTrack] = null + } + } + uncommittedReactionRefs.clear() + } + + if (reactionCleanupHandle) { + clearTimeout(reactionCleanupHandle) + reactionCleanupHandle = undefined + } + } + + function ensureCleanupTimerRunning() { + if (reactionCleanupHandle === undefined) { + reactionCleanupHandle = setTimeout(cleanUncommittedReactions, CLEANUP_TIMER_LOOP_MILLIS) + } + } + + function scheduleCleanupOfReactionIfLeaked(ref: Comp) { + uncommittedReactionRefs.add(ref) + + ensureCleanupTimerRunning() + } + + function recordReactionAsCommitted(reactionRef: Comp) { + uncommittedReactionRefs.delete(reactionRef) + } + + /** + * Run by the cleanup timer to dispose any outstanding reactions + */ + function cleanUncommittedReactions() { + reactionCleanupHandle = undefined + + // Loop through all the candidate leaked reactions; those older + // than CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS get tidied. + + const now = Date.now() + uncommittedReactionRefs.forEach(ref => { + const tracking = ref[reactionTrack] + if (tracking) { + if (now >= tracking.cleanAt) { + // It's time to tidy up this leaked reaction. + tracking.reaction.dispose() + ref[reactionTrack] = null + uncommittedReactionRefs.delete(ref) + } + } + }) + + if (uncommittedReactionRefs.size > 0) { + // We've just finished a round of cleanups but there are still + // some leak candidates outstanding. + ensureCleanupTimerRunning() + } + } + + return { + addReactionToTrack( + reactionTrackingRef: Comp, + reaction: Reaction, + /** + * On timer based implementation we don't really need this object, + * but we keep the same api + */ + objectRetainedByReact: unknown + ) { + reactionTrackingRef.current = createTrackingData(reaction) + scheduleCleanupOfReactionIfLeaked(reactionTrackingRef) + return reactionTrackingRef.current + }, + recordReactionAsCommitted, + forceCleanupTimerToRunNowForTests, + resetCleanupScheduleForTests + } +} diff --git a/packages/mobx-react/src/utils/reactionCleanupTracking.ts b/packages/mobx-react/src/utils/reactionCleanupTracking.ts new file mode 100644 index 0000000000..33301f65f7 --- /dev/null +++ b/packages/mobx-react/src/utils/reactionCleanupTracking.ts @@ -0,0 +1,20 @@ +import { FinalizationRegistry as FinalizationRegistryMaybeUndefined } from "./FinalizationRegistryWrapper" +import { createReactionCleanupTrackingUsingFinalizationRegister } from "./createReactionCleanupTrackingUsingFinalizationRegister" +import { createTimerBasedReactionCleanupTracking } from "./createTimerBasedReactionCleanupTracking" +export { IReactionTracking, reactionTrack } from "./reactionCleanupTrackingCommon" + +const { + addReactionToTrack, + recordReactionAsCommitted, + resetCleanupScheduleForTests, + forceCleanupTimerToRunNowForTests +} = FinalizationRegistryMaybeUndefined + ? createReactionCleanupTrackingUsingFinalizationRegister(FinalizationRegistryMaybeUndefined) + : createTimerBasedReactionCleanupTracking() + +export { + addReactionToTrack, + recordReactionAsCommitted, + resetCleanupScheduleForTests, + forceCleanupTimerToRunNowForTests +} diff --git a/packages/mobx-react/src/utils/reactionCleanupTrackingCommon.ts b/packages/mobx-react/src/utils/reactionCleanupTrackingCommon.ts new file mode 100644 index 0000000000..6d191f9a07 --- /dev/null +++ b/packages/mobx-react/src/utils/reactionCleanupTrackingCommon.ts @@ -0,0 +1,77 @@ +import { Reaction } from "mobx" +import { newSymbol } from "./utils" + +export const reactionTrack = newSymbol("reactionTrack") + +export type Comp = { [key: typeof reactionTrack]: IReactionTracking | null } + +export function createTrackingData(reaction: Reaction) { + const trackingData: IReactionTracking = { + reaction, + mounted: false, + changedBeforeMount: false, + cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS + } + return trackingData +} + +/** + * Unified api for timers/Finalization registry cleanups + * This abstraction make useObserver much simpler + */ +export interface ReactionCleanupTracking { + /** + * + * @param reaction The reaction to cleanup + * @param objectRetainedByReact This will be in actual use only when FinalizationRegister is in use + */ + addReactionToTrack( + reactionTrackingRef: Comp, + reaction: Reaction, + objectRetainedByReact: object + ): IReactionTracking + recordReactionAsCommitted(reactionRef: Comp): void + forceCleanupTimerToRunNowForTests(): void + resetCleanupScheduleForTests(): void +} + +export interface IReactionTracking { + /** The Reaction created during first render, which may be leaked */ + reaction: Reaction + /** + * The time (in ticks) at which point we should dispose of the reaction + * if this component hasn't yet been fully mounted. + */ + cleanAt: number + + /** + * Whether the component has yet completed mounting (for us, whether + * its useEffect has run) + */ + mounted: boolean + + /** + * Whether the observables that the component is tracking changed between + * the first render and the first useEffect. + */ + changedBeforeMount: boolean + + /** + * In case we are using finalization registry based cleanup, + * this will hold the cleanup token associated with this reaction + */ + finalizationRegistryCleanupToken?: number +} + +/** + * The minimum time before we'll clean up a Reaction created in a render + * for a component that hasn't managed to run its effects. This needs to + * be big enough to ensure that a component won't turn up and have its + * effects run without being re-rendered. + */ +export const CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS = 10_000 + +/** + * The frequency with which we'll check for leaked reactions. + */ +export const CLEANUP_TIMER_LOOP_MILLIS = 10_000