From 589086b5af8a57fc5d1c7253f880558165f88c33 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Thu, 19 Oct 2023 20:03:42 +0200 Subject: [PATCH 01/11] fix(mobx-react-lit): dispose reactions right after render in StrictMode and Suspense onBecomeUnobserved never triggered with observer component mobxjs/mobx#3774 --- .../strictAndConcurrentMode.test.tsx | 51 +++++++++++++++++++ ...rentModeUsingFinalizationRegistry.test.tsx | 3 ++ ...trictAndConcurrentModeUsingTimers.test.tsx | 3 ++ .../utils/RequestAnimationFrameMockSession.ts | 35 +++++++++++++ packages/mobx-react-lite/src/useObserver.ts | 41 +++++++++++---- 5 files changed, 122 insertions(+), 11 deletions(-) create mode 100644 packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx index 519fd1193..e40a125f7 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx +++ b/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 | 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( + + + + ) + 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() +}) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index d10b894fd..5dc35355c 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/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 () => { ) + requestAnimationFrameMock.triggerAllAnimationFrames() rendering.rerender( ) + requestAnimationFrameMock.triggerAllAnimationFrames() // Allow gc to kick in in case to let finalization registry cleanup await new Promise(resolve => setTimeout(resolve, 100)) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx index d7250ccb4..a5183b5d0 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx +++ b/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 () => { ) + requestAnimationFrameMock.triggerAllAnimationFrames() rendering.rerender( ) + requestAnimationFrameMock.triggerAllAnimationFrames() // Allow any reaction-disposal cleanup timers to run const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) diff --git a/packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts b/packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts new file mode 100644 index 000000000..928f76014 --- /dev/null +++ b/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) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 8bc07b360..de2fd7b48 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/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(render: () => T, baseComponentName: string = "observed"): T { if (isUsingStaticRendering()) { return render() } + const animationRequestIDRef = React.useRef(null) const admRef = React.useRef(null) if (!admRef.current) { @@ -50,7 +56,6 @@ export function useObserver(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(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(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(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 } From d2698a400367f3da7a7f8ad8e4e8cc2c1a8d8b15 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Thu, 19 Oct 2023 20:15:15 +0200 Subject: [PATCH 02/11] chore(mobx-react-lite): test description --- .../mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx index e40a125f7..2b47e7f00 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx @@ -67,7 +67,7 @@ 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 => { +test("destroy reaction in the next animation frame if component destroyed", async doneCallback => { const o = mobx.observable({ x: 0, promise: null as Promise | null }) const Cmp = () => useObserver(() => { From 38dfb2cb24800db69778efd8d8359459d71d3df2 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Thu, 19 Oct 2023 21:08:41 +0200 Subject: [PATCH 03/11] chore(mobx-react-lite): simplify useObserver and update comments --- packages/mobx-react-lite/src/useObserver.ts | 74 ++++++++++----------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index de2fd7b48..4367711f9 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -7,8 +7,6 @@ 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 = { reaction: Reaction | null // also serves as disposed flag onStoreChange: Function | null // also serves as mounted flag @@ -46,53 +44,72 @@ export function useObserver(render: () => T, baseComponentName: string = "obs const animationRequestIDRef = React.useRef(null) const admRef = React.useRef(null) + let adm = admRef.current - if (!admRef.current) { + if (!adm) { // First render - const adm: ObserverAdministration = { + adm = { reaction: null, onStoreChange: null, stateVersion: Symbol(), name: baseComponentName, subscribe(onStoreChange: () => void) { - // Do NOT access admRef here! - adm.onStoreChange = onStoreChange - if (!adm.reaction) { + this.onStoreChange = onStoreChange + if (!this.reaction) { // We've lost our reaction and therefore all subscriptions, occurs when: - // 1. Timer based finalization registry disposed reaction before component mounted. + // 1. requestAnimationFrame 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) + createReaction(this) // `onStoreChange` won't force update if subsequent `getSnapshot` returns same value. // So we make sure that is not the case - adm.stateVersion = Symbol() + this.stateVersion = Symbol() } return () => { - // Do NOT access admRef here! - disposeReaction(adm) + disposeReaction(this) } }, getSnapshot() { - // Do NOT access admRef here! - return adm.stateVersion + return this.stateVersion } } + adm.subscribe = adm.subscribe.bind(adm) + adm.getSnapshot = adm.getSnapshot.bind(adm) + admRef.current = adm } - const adm = admRef.current! - const firstRender = !adm.reaction - - if (firstRender) { - // First render or reaction was disposed by registry before subscribe + if (!adm.reaction) { + // First render or reaction was disposed before subscribe createReaction(adm) } React.useDebugValue(adm.reaction!, printDebugValue) + if (animationRequestIDRef.current !== null) { + // cancel previous animation frame + cancelAnimationFrame(animationRequestIDRef.current) + animationRequestIDRef.current = null + } + + animationRequestIDRef.current = requestAnimationFrame(() => { + // 1. StrictMode/ConcurrentMode/Suspense may mean that our component is + // rendered and abandoned multiple times, so we need to dispose leaked + // Reactions. + // 2. The component haven't been rendered in the following animation frame. + disposeReaction(adm!) + }) + + const animationRequestID = animationRequestIDRef.current + + useLayoutEffect(() => { + // Component mounted, we don't need to dispose reaction anymore + cancelAnimationFrame(animationRequestID) + }) + useSyncExternalStore( // Both of these must be stable, otherwise it would keep resubscribing every render. adm.subscribe, @@ -113,28 +130,9 @@ export function useObserver(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 } From ff14583bfd97101551cdc489d84ffa65dde0e5a5 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Thu, 19 Oct 2023 22:00:58 +0200 Subject: [PATCH 04/11] tests(mobx-react-lite): remove tests based on FinalizationRegistry --- .../strictAndConcurrentMode.test.tsx | 37 ++++ ...rentModeUsingFinalizationRegistry.test.tsx | 63 ------ ...trictAndConcurrentModeUsingTimers.test.tsx | 196 ------------------ 3 files changed, 37 insertions(+), 259 deletions(-) delete mode 100644 packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx delete mode 100644 packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx index 2b47e7f00..3952b0601 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx @@ -7,6 +7,7 @@ import { useObserver } from "../src/useObserver" import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession" afterEach(cleanup) +afterEach(() => requestAnimationFrameMock.reset()) test("uncommitted observing components should not attempt state changes", () => { const store = mobx.observable({ count: 0 }) @@ -116,3 +117,39 @@ test("destroy reaction in the next animation frame if component destroyed", asyn doneCallback() }) + +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}
) + + // Render, then remove only #2 + const rendering = render( + + + + + ) + rendering.rerender( + + + + ) + + // Force reactions to be disposed + requestAnimationFrameMock.triggerAllAnimationFrames() + + // count1 should still be being observed by Component1, + // but count2 should have had its reaction cleaned up. + expect(count1IsObserved).toBeTruthy() + expect(count2IsObserved).toBeFalsy() +}) 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 5dc35355c..000000000 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ /dev/null @@ -1,63 +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" -import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession" - -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( - - - - - ) - requestAnimationFrameMock.triggerAllAnimationFrames() - rendering.rerender( - - - - ) - requestAnimationFrameMock.triggerAllAnimationFrames() - - // 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 a5183b5d0..000000000 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx +++ /dev/null @@ -1,196 +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" -import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession" - -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( - - - - - ) - requestAnimationFrameMock.triggerAllAnimationFrames() - rendering.rerender( - - - - ) - requestAnimationFrameMock.triggerAllAnimationFrames() - - // 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") -}) From 3df6892fc06d030e00108042c408b6a7b3c96005 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Fri, 20 Oct 2023 00:02:13 +0200 Subject: [PATCH 05/11] refactor(mobx-react-lite): split useObserver logic to independent parts --- packages/mobx-react-lite/src/useObserver.ts | 138 +++++++++++--------- 1 file changed, 75 insertions(+), 63 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 4367711f9..c6d503e63 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -15,79 +15,30 @@ type ObserverAdministration = { // 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() - // 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?.() - }) +function createReaction(name: string, adm: ObserverAdministration) { + adm.reaction = new Reaction(`observer${name}`, () => forceUpdate(adm)) } function disposeReaction(adm: ObserverAdministration) { - adm.onStoreChange = null adm.reaction?.dispose() adm.reaction = null } -export function useObserver(render: () => T, baseComponentName: string = "observed"): T { - if (isUsingStaticRendering()) { - return render() - } +function forceUpdate(adm: ObserverAdministration) { + adm.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?.() +} +function useReactionDisposer(adm: ObserverAdministration) { const animationRequestIDRef = React.useRef(null) - const admRef = React.useRef(null) - let adm = admRef.current - - if (!adm) { - // First render - adm = { - reaction: null, - onStoreChange: null, - stateVersion: Symbol(), - name: baseComponentName, - subscribe(onStoreChange: () => void) { - this.onStoreChange = onStoreChange - if (!this.reaction) { - // We've lost our reaction and therefore all subscriptions, occurs when: - // 1. requestAnimationFrame 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(this) - // `onStoreChange` won't force update if subsequent `getSnapshot` returns same value. - // So we make sure that is not the case - this.stateVersion = Symbol() - } - - return () => { - disposeReaction(this) - } - }, - getSnapshot() { - return this.stateVersion - } - } - - adm.subscribe = adm.subscribe.bind(adm) - adm.getSnapshot = adm.getSnapshot.bind(adm) - - admRef.current = adm - } - - if (!adm.reaction) { - // First render or reaction was disposed before subscribe - createReaction(adm) - } - - React.useDebugValue(adm.reaction!, printDebugValue) if (animationRequestIDRef.current !== null) { // cancel previous animation frame @@ -101,14 +52,75 @@ export function useObserver(render: () => T, baseComponentName: string = "obs // Reactions. // 2. The component haven't been rendered in the following animation frame. disposeReaction(adm!) + animationRequestIDRef.current = null }) - const animationRequestID = animationRequestIDRef.current - useLayoutEffect(() => { - // Component mounted, we don't need to dispose reaction anymore - cancelAnimationFrame(animationRequestID) + if (animationRequestIDRef.current !== null) { + // Component mounted, we don't need to dispose reaction anymore + cancelAnimationFrame(animationRequestIDRef.current) + } + + // In some rare cases reaction will be disposed before component mounted, + // but we still need to recreate it. + if (adm && !adm.reaction) { + forceUpdate(adm) + } }) +} + +function createObserverAdministration(): ObserverAdministration { + const adm: ObserverAdministration = { + reaction: null, + onStoreChange: null, + stateVersion: Symbol(), + subscribe(onStoreChange: () => void) { + this.onStoreChange = onStoreChange + if (!this.reaction) { + // We've lost our reaction and therefore all subscriptions, occurs when: + // 1. requestAnimationFrame disposed reaction before component mounted. + // 2. React "re-mounts" same component without calling render in between (typically ). + // We have to schedule re-render to recreate reaction and subscriptions, even if state did not change. + forceUpdate(this) + } + + return () => { + this.onStoreChange = null + disposeReaction(this) + } + }, + getSnapshot() { + return this.stateVersion + } + } + + adm.subscribe = adm.subscribe.bind(adm) + adm.getSnapshot = adm.getSnapshot.bind(adm) + + return adm +} + +export function useObserver(render: () => T, baseComponentName: string = "observed"): T { + if (isUsingStaticRendering()) { + return render() + } + + const admRef = React.useRef(null) + let adm = admRef.current + + if (!adm) { + // First render + admRef.current = adm = createObserverAdministration() + } + + if (!adm.reaction) { + // First render or reaction was disposed before subscribe + createReaction(baseComponentName, adm) + } + + React.useDebugValue(adm.reaction!, printDebugValue) + + useReactionDisposer(adm) useSyncExternalStore( // Both of these must be stable, otherwise it would keep resubscribing every render. From 2895cdf0d5605888b69f3ae0d4f0c63d3e3c1418 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Fri, 20 Oct 2023 00:22:49 +0200 Subject: [PATCH 06/11] chore(mobx-react-lite): better typing in the useObserver --- packages/mobx-react-lite/src/useObserver.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index c6d503e63..745876c96 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -1,5 +1,5 @@ import { Reaction } from "mobx" -import React, { useLayoutEffect } from "react" +import React from "react" import { printDebugValue } from "./utils/printDebugValue" import { isUsingStaticRendering } from "./staticRendering" import { useSyncExternalStore } from "use-sync-external-store/shim" @@ -20,8 +20,8 @@ type ObserverAdministration = { getSnapshot: Parameters[1] } -function createReaction(name: string, adm: ObserverAdministration) { - adm.reaction = new Reaction(`observer${name}`, () => forceUpdate(adm)) +function createReaction(name: string, adm: ObserverAdministration): Reaction { + return new Reaction(`observer${name}`, () => forceUpdate(adm)) } function disposeReaction(adm: ObserverAdministration) { @@ -55,7 +55,7 @@ function useReactionDisposer(adm: ObserverAdministration) { animationRequestIDRef.current = null }) - useLayoutEffect(() => { + React.useLayoutEffect(() => { if (animationRequestIDRef.current !== null) { // Component mounted, we don't need to dispose reaction anymore cancelAnimationFrame(animationRequestIDRef.current) @@ -110,15 +110,15 @@ export function useObserver(render: () => T, baseComponentName: string = "obs if (!adm) { // First render - admRef.current = adm = createObserverAdministration() + adm = admRef.current = createObserverAdministration() } if (!adm.reaction) { // First render or reaction was disposed before subscribe - createReaction(baseComponentName, adm) + adm.reaction = createReaction(baseComponentName, adm) } - React.useDebugValue(adm.reaction!, printDebugValue) + React.useDebugValue(adm.reaction, printDebugValue) useReactionDisposer(adm) @@ -134,7 +134,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs // can be invalidated (see above) once a dependency changes let renderResult!: T let exception - adm.reaction!.track(() => { + adm.reaction.track(() => { try { renderResult = render() } catch (e) { From 77fef6319b12649e61a44ab2782313bf3a2239ba Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Fri, 20 Oct 2023 00:47:52 +0200 Subject: [PATCH 07/11] fix(mobx-react-lite): reset state properly --- packages/mobx-react-lite/src/useObserver.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 745876c96..e2e95ef55 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -59,6 +59,7 @@ function useReactionDisposer(adm: ObserverAdministration) { if (animationRequestIDRef.current !== null) { // Component mounted, we don't need to dispose reaction anymore cancelAnimationFrame(animationRequestIDRef.current) + animationRequestIDRef.current = null } // In some rare cases reaction will be disposed before component mounted, From 7531b6f6bae152c284d3d3a4a5b7fb0a63b06286 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Sat, 21 Oct 2023 01:37:52 +0200 Subject: [PATCH 08/11] fix(mobx-react-lite): use setTimeout for disposing abandoned reactions --- .../strictAndConcurrentMode.test.tsx | 104 +++++++++---- .../__tests__/utils/ErrorBoundary.ts | 23 +++ .../utils/RequestAnimationFrameMockSession.ts | 35 ----- packages/mobx-react-lite/src/useObserver.ts | 141 +++++++----------- 4 files changed, 154 insertions(+), 149 deletions(-) create mode 100644 packages/mobx-react-lite/__tests__/utils/ErrorBoundary.ts delete mode 100644 packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx index 3952b0601..75e4775fc 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx @@ -1,13 +1,12 @@ -import { act, cleanup, render } from "@testing-library/react" +import { act, cleanup, render, waitFor } 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" -import { requestAnimationFrameMock } from "./utils/RequestAnimationFrameMockSession" afterEach(cleanup) -afterEach(() => requestAnimationFrameMock.reset()) test("uncommitted observing components should not attempt state changes", () => { const store = mobx.observable({ count: 0 }) @@ -68,15 +67,17 @@ test(`observable changes before first commit are not lost`, async () => { expect(rendering.baseElement.textContent).toBe("changed") }) -test("destroy reaction in the next animation frame if component destroyed", async doneCallback => { +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 as any + + return <>{o.x} }) const observed = jest.fn() @@ -84,38 +85,52 @@ test("destroy reaction in the next animation frame if component destroyed", asyn mobx.onBecomeObserved(o, "x", observed) mobx.onBecomeUnobserved(o, "x", unobserved) + jest.useFakeTimers() const { container, unmount } = render( ) - requestAnimationFrameMock.triggerAllAnimationFrames() + jest.runAllTimers() expect(container).toHaveTextContent("0") expect(observed).toBeCalledTimes(1) - expect(unobserved).toBeCalledTimes(0) - act( - mobx.action(() => { - o.promise = Promise.resolve() - }) - ) - requestAnimationFrameMock.triggerAllAnimationFrames() + + let resolve: () => void + act(() => { + o.promise = new Promise(r => (resolve = r)) + }) + + jest.runAllTimers() 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) + 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() - doneCallback() + jest.runAllTimers() + expect(observed).toBeCalledTimes(1) + expect(unobserved).toBeCalledTimes(1) + jest.useRealTimers() }) test("uncommitted components should not leak observations", async () => { @@ -132,6 +147,7 @@ test("uncommitted components should not leak observations", async () => { const TestComponent1 = () => useObserver(() =>
{store.count1}
) const TestComponent2 = () => useObserver(() =>
{store.count2}
) + jest.useFakeTimers() // Render, then remove only #2 const rendering = render( @@ -145,11 +161,41 @@ test("uncommitted components should not leak observations", async () => { ) - // Force reactions to be disposed - requestAnimationFrameMock.triggerAllAnimationFrames() - + 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__/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/__tests__/utils/RequestAnimationFrameMockSession.ts b/packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts deleted file mode 100644 index 928f76014..000000000 --- a/packages/mobx-react-lite/__tests__/utils/RequestAnimationFrameMockSession.ts +++ /dev/null @@ -1,35 +0,0 @@ -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) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index e2e95ef55..5b95ff241 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -7,98 +7,76 @@ import { useSyncExternalStore } from "use-sync-external-store/shim" // Required by SSR when hydrating #3669 const getServerSnapshot = () => {} -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 - // These don't depend on state/props, therefore we can keep them here instead of `useCallback` - subscribe: Parameters[0] - getSnapshot: Parameters[1] -} -function createReaction(name: string, adm: ObserverAdministration): Reaction { - return new Reaction(`observer${name}`, () => forceUpdate(adm)) -} + 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) -function disposeReaction(adm: ObserverAdministration) { - adm.reaction?.dispose() - adm.reaction = null -} + this.reaction = new Reaction(`observer${name}`, this.forceUpdate) + this.onStoreChange = null + this.stateVersion = Symbol() + this.timeoutID = null -function forceUpdate(adm: ObserverAdministration) { - adm.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.scheduleDispose() + } -function useReactionDisposer(adm: ObserverAdministration) { - const animationRequestIDRef = React.useRef(null) + subscribe(onStoreChange: () => void) { + this.cancelDispose() + this.onStoreChange = onStoreChange + if (!this.reaction) { + // We've lost our reaction and therefore all subscriptions, occurs when: + // 1. requestAnimationFrame disposed reaction before component mounted. + // 2. React "re-mounts" same component without calling render in between (typically ). + // We have to schedule re-render to recreate reaction and subscriptions, even if state did not change. + this.forceUpdate() + } - if (animationRequestIDRef.current !== null) { - // cancel previous animation frame - cancelAnimationFrame(animationRequestIDRef.current) - animationRequestIDRef.current = null + return this.dispose } - animationRequestIDRef.current = requestAnimationFrame(() => { - // 1. StrictMode/ConcurrentMode/Suspense may mean that our component is - // rendered and abandoned multiple times, so we need to dispose leaked - // Reactions. - // 2. The component haven't been rendered in the following animation frame. - disposeReaction(adm!) - animationRequestIDRef.current = null - }) - - React.useLayoutEffect(() => { - if (animationRequestIDRef.current !== null) { - // Component mounted, we don't need to dispose reaction anymore - cancelAnimationFrame(animationRequestIDRef.current) - animationRequestIDRef.current = null - } + getSnapshot() { + return this.stateVersion + } - // In some rare cases reaction will be disposed before component mounted, - // but we still need to recreate it. - if (adm && !adm.reaction) { - forceUpdate(adm) - } - }) -} + 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. + this.onStoreChange?.() + } -function createObserverAdministration(): ObserverAdministration { - const adm: ObserverAdministration = { - reaction: null, - onStoreChange: null, - stateVersion: Symbol(), - subscribe(onStoreChange: () => void) { - this.onStoreChange = onStoreChange - if (!this.reaction) { - // We've lost our reaction and therefore all subscriptions, occurs when: - // 1. requestAnimationFrame disposed reaction before component mounted. - // 2. React "re-mounts" same component without calling render in between (typically ). - // We have to schedule re-render to recreate reaction and subscriptions, even if state did not change. - forceUpdate(this) - } - - return () => { - this.onStoreChange = null - disposeReaction(this) - } - }, - getSnapshot() { - return this.stateVersion - } + private dispose() { + this.reaction?.dispose() + this.reaction = null + this.onStoreChange = null + this.cancelDispose() } - adm.subscribe = adm.subscribe.bind(adm) - adm.getSnapshot = adm.getSnapshot.bind(adm) + private scheduleDispose() { + this.timeoutID = setTimeout(() => this.dispose(), DISPOSE_TIMEOUT) as unknown as number + } - return adm + private cancelDispose() { + if (this.timeoutID !== null) { + clearTimeout(this.timeoutID) + this.timeoutID = null + } + } } export function useObserver(render: () => T, baseComponentName: string = "observed"): T { @@ -109,19 +87,12 @@ export function useObserver(render: () => T, baseComponentName: string = "obs const admRef = React.useRef(null) let adm = admRef.current - if (!adm) { - // First render - adm = admRef.current = createObserverAdministration() + if (!adm?.reaction) { + // First render or reaction was disposed + adm = admRef.current = new ObserverAdministration(baseComponentName) } - if (!adm.reaction) { - // First render or reaction was disposed before subscribe - adm.reaction = createReaction(baseComponentName, adm) - } - - React.useDebugValue(adm.reaction, printDebugValue) - - useReactionDisposer(adm) + React.useDebugValue(adm.reaction!, printDebugValue) useSyncExternalStore( // Both of these must be stable, otherwise it would keep resubscribing every render. @@ -135,7 +106,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs // can be invalidated (see above) once a dependency changes let renderResult!: T let exception - adm.reaction.track(() => { + adm.reaction!.track(() => { try { renderResult = render() } catch (e) { From 58889d38c3afaf4f1886415bc2dffc99f7cd7510 Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Sat, 21 Oct 2023 01:58:36 +0200 Subject: [PATCH 09/11] chore(mobx-react-lite): remove unused imports --- .../mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx index 75e4775fc..71ec4fc38 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentMode.test.tsx @@ -1,4 +1,4 @@ -import { act, cleanup, render, waitFor } from "@testing-library/react" +import { act, cleanup, render } from "@testing-library/react" import mockConsole from "jest-mock-console" import * as mobx from "mobx" import * as React from "react" From b9ca9862ba8753230ce2415e9ab8134a380034ca Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Sun, 22 Oct 2023 13:39:25 +0200 Subject: [PATCH 10/11] fix(mobx-react-lite): schedule component re-render on reaction dispose --- packages/mobx-react-lite/src/useObserver.ts | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 5b95ff241..e0a3689c5 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -37,15 +37,11 @@ class ObserverAdministration { subscribe(onStoreChange: () => void) { this.cancelDispose() this.onStoreChange = onStoreChange - if (!this.reaction) { - // We've lost our reaction and therefore all subscriptions, occurs when: - // 1. requestAnimationFrame disposed reaction before component mounted. - // 2. React "re-mounts" same component without calling render in between (typically ). - // We have to schedule re-render to recreate reaction and subscriptions, even if state did not change. - this.forceUpdate() - } - return this.dispose + return () => { + this.onStoreChange = null + this.dispose() + } } getSnapshot() { @@ -61,6 +57,13 @@ class ObserverAdministration { } 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 ). + // 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 @@ -68,7 +71,7 @@ class ObserverAdministration { } private scheduleDispose() { - this.timeoutID = setTimeout(() => this.dispose(), DISPOSE_TIMEOUT) as unknown as number + this.timeoutID = setTimeout(this.dispose, DISPOSE_TIMEOUT) as unknown as number } private cancelDispose() { From 94bc4997c14152ff5aefcaac64d982d5c21ba51a Mon Sep 17 00:00:00 2001 From: Aleksey Potsetsuev Date: Sun, 22 Oct 2023 13:42:28 +0200 Subject: [PATCH 11/11] chore(mobx-react-lite): update comments --- packages/mobx-react-lite/src/useObserver.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index e0a3689c5..e00e33b00 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -60,6 +60,7 @@ class ObserverAdministration { // 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()