diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx index 519fd1193..71ec4fc38 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx @@ -2,6 +2,7 @@ import { act, cleanup, render } from "@testing-library/react" import mockConsole from "jest-mock-console" import * as mobx from "mobx" import * as React from "react" +import { ErrorBoundary } from "./utils/ErrorBoundary" import { useObserver } from "../src/useObserver" @@ -65,3 +66,136 @@ test(`observable changes before first commit are not lost`, async () => { expect(rendering.baseElement.textContent).toBe("changed") }) + +test("suspended components should not leak observations", () => { + const o = mobx.observable({ x: 0, promise: null as Promise | null }) + const Cmp = () => + useObserver(() => { + o.x as any // establish dependency + + if (o.promise) { + throw o.promise + } + + return <>{o.x} + }) + + const observed = jest.fn() + const unobserved = jest.fn() + mobx.onBecomeObserved(o, "x", observed) + mobx.onBecomeUnobserved(o, "x", unobserved) + + jest.useFakeTimers() + const { container, unmount } = render( + + + + ) + + jest.runAllTimers() + expect(container).toHaveTextContent("0") + expect(observed).toBeCalledTimes(1) + + let resolve: () => void + act(() => { + o.promise = new Promise(r => (resolve = r)) + }) + + jest.runAllTimers() + expect(container).toHaveTextContent("loading...") + expect(observed).toBeCalledTimes(1) + expect(unobserved).toBeCalledTimes(0) + + act(() => { + o.promise = null + resolve!() + }) + + jest.runAllTimers() + expect(container).toHaveTextContent(o.x + "") + + // ensure that we using same reaction and component state + expect(observed).toBeCalledTimes(1) + expect(unobserved).toBeCalledTimes(0) + + act(() => { + o.x++ + }) + + jest.runAllTimers() + expect(container).toHaveTextContent(o.x + "") + + unmount() + + jest.runAllTimers() + expect(observed).toBeCalledTimes(1) + expect(unobserved).toBeCalledTimes(1) + jest.useRealTimers() +}) + +test("uncommitted components should not leak observations", async () => { + 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)) + + const TestComponent1 = () => useObserver(() =>
{store.count1}
) + const TestComponent2 = () => useObserver(() =>
{store.count2}
) + + jest.useFakeTimers() + // Render, then remove only #2 + const rendering = render( + + + + + ) + rendering.rerender( + + + + ) + + jest.runAllTimers() + // count1 should still be being observed by Component1, + // but count2 should have had its reaction cleaned up. + expect(count1IsObserved).toBeTruthy() + expect(count2IsObserved).toBeFalsy() + + jest.useRealTimers() +}) + +test("abandoned components should not leak observations", async () => { + const store = mobx.observable({ count: 0 }) + + let countIsObserved = false + mobx.onBecomeObserved(store, "count", () => (countIsObserved = true)) + mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false)) + + const TestComponent = () => + useObserver(() => { + store.count // establish dependency + throw new Error("not rendered") + }) + + jest.useFakeTimers() + + render( + + + + ) + + expect(countIsObserved).toBeTruthy() + + jest.runAllTimers() + + expect(countIsObserved).toBeFalsy() + + jest.useRealTimers() +}) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx deleted file mode 100644 index d10b894fd..000000000 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ /dev/null @@ -1,60 +0,0 @@ -import { cleanup, render, waitFor } from "@testing-library/react" -import * as mobx from "mobx" -import * as React from "react" -import { useObserver } from "../src/useObserver" -import gc from "expose-gc/function" -import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" - -if (typeof globalThis.FinalizationRegistry !== "function") { - throw new Error("This test must run with node >= 14") -} - -expect(observerFinalizationRegistry).toBeInstanceOf(globalThis.FinalizationRegistry) - -afterEach(cleanup) - -test("uncommitted components should not leak observations", async () => { - jest.setTimeout(30_000) - 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)) - - const TestComponent1 = () => useObserver(() =>
{store.count1}
) - const TestComponent2 = () => useObserver(() =>
{store.count2}
) - - // Render, then remove only #2 - const rendering = render( - - - - - ) - rendering.rerender( - - - - ) - - // Allow gc to kick in in case to let finalization registry cleanup - await new Promise(resolve => setTimeout(resolve, 100)) - gc() - // Can take a while (especially on CI) before gc actually calls the registry - await waitFor( - () => { - // count1 should still be being observed by Component1, - // but count2 should have had its reaction cleaned up. - expect(count1IsObserved).toBeTruthy() - expect(count2IsObserved).toBeFalsy() - }, - { - timeout: 15_000, - interval: 200 - } - ) -}) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx deleted file mode 100644 index d7250ccb4..000000000 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx +++ /dev/null @@ -1,193 +0,0 @@ -import "./utils/killFinalizationRegistry" -import { act, cleanup, render } from "@testing-library/react" -import * as mobx from "mobx" -import * as React from "react" -import { useObserver } from "../src/useObserver" -import { - REGISTRY_FINALIZE_AFTER, - REGISTRY_SWEEP_INTERVAL -} from "../src/utils/UniversalFinalizationRegistry" -import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" -import { TimerBasedFinalizationRegistry } from "../src/utils/UniversalFinalizationRegistry" - -expect(observerFinalizationRegistry).toBeInstanceOf(TimerBasedFinalizationRegistry) - -const registry = observerFinalizationRegistry as TimerBasedFinalizationRegistry - -afterEach(cleanup) - -test("uncommitted components should not leak observations", async () => { - registry.finalizeAllImmediately() - - // 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)) - - const TestComponent1 = () => useObserver(() =>
{store.count1}
) - const TestComponent2 = () => useObserver(() =>
{store.count2}
) - - // Render, then remove only #2 - const rendering = render( - - - - - ) - rendering.rerender( - - - - ) - - // Allow any reaction-disposal cleanup timers to run - const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) - 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: - registry.finalizeAllImmediately() - - // 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)) - - const TestComponent1 = () => useObserver(() =>
{store.count}
) - - 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. - registry.sweep() - - // 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: - - registry.finalizeAllImmediately() - - // 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)) - - const TestComponent1 = () => useObserver(() =>
{store.count}
) - - 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(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) - fakeNow += skip - registry.sweep() - - // 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-lite/__tests__/utils/ErrorBoundary.ts b/packages/mobx-react-lite/__tests__/utils/ErrorBoundary.ts new file mode 100644 index 000000000..cd73c8fa3 --- /dev/null +++ b/packages/mobx-react-lite/__tests__/utils/ErrorBoundary.ts @@ -0,0 +1,23 @@ +import React from "react" + +export class ErrorBoundary extends React.Component< + React.PropsWithChildren<{ fallback: string }>, + { hasError: boolean } +> { + constructor(props) { + super(props) + this.state = { hasError: false } + } + + static getDerivedStateFromError(error) { + return { hasError: true } + } + + render() { + if (this.state.hasError) { + return this.props.fallback + } + + return this.props.children + } +} diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 8bc07b360..e00e33b00 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -2,93 +2,98 @@ import { Reaction } from "mobx" import React 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 const getServerSnapshot = () => {} -// Do not store `admRef` (even as part of a closure!) on this object, -// otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry. -type ObserverAdministration = { +// will prevent disposing reaction of delayed components +const DISPOSE_TIMEOUT = 100 + +class ObserverAdministration { reaction: Reaction | null // also serves as disposed flag onStoreChange: Function | null // also serves as mounted flag + timeoutID: number | null // BC: we will use local state version if global isn't available. // It should behave as previous implementation - tearing is still present, // because there is no cross component synchronization, // but we can use `useSyncExternalStore` API. stateVersion: any - name: string - // These don't depend on state/props, therefore we can keep them here instead of `useCallback` - subscribe: Parameters[0] - getSnapshot: Parameters[1] -} -function createReaction(adm: ObserverAdministration) { - adm.reaction = new Reaction(`observer${adm.name}`, () => { - adm.stateVersion = Symbol() + constructor(name: string) { + this.forceUpdate = this.forceUpdate.bind(this) + this.subscribe = this.subscribe.bind(this) + this.getSnapshot = this.getSnapshot.bind(this) + this.dispose = this.dispose.bind(this) + + this.reaction = new Reaction(`observer${name}`, this.forceUpdate) + this.onStoreChange = null + this.stateVersion = Symbol() + this.timeoutID = null + + this.scheduleDispose() + } + + subscribe(onStoreChange: () => void) { + this.cancelDispose() + this.onStoreChange = onStoreChange + + return () => { + this.onStoreChange = null + this.dispose() + } + } + + getSnapshot() { + return this.stateVersion + } + + private forceUpdate() { + this.stateVersion = Symbol() // onStoreChange won't be available until the component "mounts". // If state changes in between initial render and mount, // `useSyncExternalStore` should handle that by checking the state version and issuing update. - adm.onStoreChange?.() - }) -} + this.onStoreChange?.() + } -export function useObserver(render: () => T, baseComponentName: string = "observed"): T { - if (isUsingStaticRendering()) { - return render() + private dispose() { + // We've lost our reaction and therefore all subscriptions, occurs when: + // 1. scheduleDispose disposed reaction before component mounted. + // 2. React "re-mounts" same component without calling render in between (typically ). + // 3. component was unmounted. + // We have to schedule re-render to recreate reaction and subscriptions, even if state did not change. + // This will have no effect if component is not mounted. + this.stateVersion = Symbol() + + this.reaction?.dispose() + this.reaction = null + this.onStoreChange = null + this.cancelDispose() } - const admRef = React.useRef(null) + private scheduleDispose() { + this.timeoutID = setTimeout(this.dispose, DISPOSE_TIMEOUT) as unknown as number + } - if (!admRef.current) { - // First render - const adm: ObserverAdministration = { - reaction: null, - onStoreChange: null, - stateVersion: Symbol(), - 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: - // 1. Timer based finalization registry disposed reaction before component mounted. - // 2. React "re-mounts" same component without calling render in between (typically ). - // We have to recreate reaction and schedule re-render to recreate subscriptions, - // even if state did not change. - createReaction(adm) - // `onStoreChange` won't force update if subsequent `getSnapshot` returns same value. - // So we make sure that is not the case - adm.stateVersion = Symbol() - } - - return () => { - // Do NOT access admRef here! - adm.onStoreChange = null - adm.reaction?.dispose() - adm.reaction = null - } - }, - getSnapshot() { - // Do NOT access admRef here! - return adm.stateVersion - } + private cancelDispose() { + if (this.timeoutID !== null) { + clearTimeout(this.timeoutID) + this.timeoutID = null } + } +} - admRef.current = adm +export function useObserver(render: () => T, baseComponentName: string = "observed"): T { + if (isUsingStaticRendering()) { + return render() } - const adm = admRef.current! + const admRef = React.useRef(null) + let adm = admRef.current - if (!adm.reaction) { - // 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) + if (!adm?.reaction) { + // First render or reaction was disposed + adm = admRef.current = new ObserverAdministration(baseComponentName) } React.useDebugValue(adm.reaction!, printDebugValue)