From 44ee8c203c84cc8f36fdefc10ec72c2fecb82bd5 Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Wed, 3 Apr 2019 12:56:29 +0100 Subject: [PATCH 01/11] Test cleaning up reactions for uncommitted components --- test/useObserver.test.tsx | 41 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/useObserver.test.tsx b/test/useObserver.test.tsx index 2e8be20c..a66ab434 100644 --- a/test/useObserver.test.tsx +++ b/test/useObserver.test.tsx @@ -4,6 +4,7 @@ import * as React from "react" import { act, cleanup, render } from "react-testing-library" import { useObserver } from "../src" +import { resetCleanupScheduleForTests } from "../src/useObserver" afterEach(cleanup) @@ -40,3 +41,43 @@ test("uncommitted observing components should not attempt state changes", () => restoreConsole() } }) + +test("uncommitted components should not leak observations", async () => { + resetCleanupScheduleForTests() + + jest.useFakeTimers() + + 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 + 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() +}) From 67d08c6529e9158d7be833bfaebacfdab20b52ca Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Wed, 3 Apr 2019 12:56:53 +0100 Subject: [PATCH 02/11] First attempt at a fix for cleaning up reactions from uncommitted components --- src/useObserver.ts | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/useObserver.ts b/src/useObserver.ts index cfeaa648..97ca2361 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -36,11 +36,13 @@ export function useObserver( forceUpdate() } }) + scheduleCleanupOfReactionIfNotCommitted(reaction.current) } useDebugValue(reaction, printDebugValue) useEffect(() => { + recordReactionAsCommitted(reaction.current!) committed.current = true return () => reaction.current!.dispose() }, []) @@ -63,3 +65,53 @@ export function useObserver( } return rendering } + +/** + * Reactions created by components that have yet to be committed. + */ +const uncommittedReactions: Reaction[] = [] + +/** + * Latest 'uncommitted reactions' cleanup timer handle + */ +let reactionCleanupHandle: number | undefined + +function scheduleCleanupOfReactionIfNotCommitted(reaction: Reaction) { + uncommittedReactions.push(reaction) + if (!reactionCleanupHandle) { + // We currently have no cleanup timer running; schedule one + reactionCleanupHandle = window.setTimeout(cleanUncommittedReactions, 100) + } +} + +function recordReactionAsCommitted(reaction: Reaction) { + // It would be more efficient if we could use a Set instead of an Array, + // but mobx-react-lite currently supports MobX 4 and ES5, so we can't assume + // the presence of Set. + const i = uncommittedReactions.indexOf(reaction) + if (i) { + uncommittedReactions.splice(i, 1) + } +} + +/** + * Run by the cleanup timer to dispose any outstanding reactions + */ +function cleanUncommittedReactions() { + reactionCleanupHandle = undefined + + while (uncommittedReactions.length) { + uncommittedReactions.pop()!.dispose() + } +} + +/** + * Only to be used by test functions; do not export outside of mobx-react-lite + */ +export function resetCleanupScheduleForTests() { + if (reactionCleanupHandle) { + clearTimeout(reactionCleanupHandle) + reactionCleanupHandle = undefined + } + uncommittedReactions.length = 0 +} From e055f95cf32656c6a0ff7d38c99187de9e62d632 Mon Sep 17 00:00:00 2001 From: Daniel K Date: Thu, 4 Apr 2019 12:16:54 +0200 Subject: [PATCH 03/11] Use debounce instead of fixed interval --- src/useObserver.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/useObserver.ts b/src/useObserver.ts index 97ca2361..a9f711f1 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -78,10 +78,11 @@ let reactionCleanupHandle: number | undefined function scheduleCleanupOfReactionIfNotCommitted(reaction: Reaction) { uncommittedReactions.push(reaction) - if (!reactionCleanupHandle) { - // We currently have no cleanup timer running; schedule one - reactionCleanupHandle = window.setTimeout(cleanUncommittedReactions, 100) + if (reactionCleanupHandle) { + window.clearTimeout(reactionCleanupHandle) } + // We currently have no cleanup timer running; schedule one + reactionCleanupHandle = window.setTimeout(cleanUncommittedReactions, 100) } function recordReactionAsCommitted(reaction: Reaction) { From 9c06f46df396ff026187d96d688a3490e9b5e29c Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Thu, 4 Apr 2019 19:54:47 +0100 Subject: [PATCH 04/11] Add unit test for missing observable changes before useEffect runs This is a test to check that observable changes made between the first component render and commit are not lost. It currently fails (and did so before the change in PR #119) --- test/useObserver.test.tsx | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/useObserver.test.tsx b/test/useObserver.test.tsx index a66ab434..d7dfc0f2 100644 --- a/test/useObserver.test.tsx +++ b/test/useObserver.test.tsx @@ -3,6 +3,7 @@ import * as mobx from "mobx" import * as React from "react" import { act, cleanup, render } from "react-testing-library" +import ReactDOM from "react-dom" import { useObserver } from "../src" import { resetCleanupScheduleForTests } from "../src/useObserver" @@ -42,6 +43,34 @@ test("uncommitted observing components should not attempt state changes", () => } }) +test("observable changes before first commit are not lost", () => { + const store = mobx.observable({ value: "initial" }) + + const TestComponent = () => useObserver(() =>
{store.value}
) + + // Render our observing component wrapped in StrictMode, but using + // raw ReactDOM.render (not react-testing-library render) because we + // don't want the useEffect calls to have run just yet... + const rootNode = document.createElement("div") + ReactDOM.render( + + + , + rootNode + ) + + // Change our observable. This is happening between the initial render of + // our component and its initial commit, so it isn't fully mounted yet. + // We want to ensure that the change isn't lost. + store.value = "changed" + + act(() => { + // no-op + }) + + expect(rootNode.textContent).toBe("changed") +}) + test("uncommitted components should not leak observations", async () => { resetCleanupScheduleForTests() From bbfe3058b0d07863ca02758033acc2ca839f0dee Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Thu, 4 Apr 2019 19:56:00 +0100 Subject: [PATCH 05/11] Add test for cleanup timer firing too early for some components This demonstrates (in a slightly contrived way) how, if the cleanup timer fires between a recent component being rendered and it being committed, that it would incorrectly tidy up a reaction for a soon-to-be-committed component. --- src/useObserver.ts | 10 +++++++ test/useObserver.test.tsx | 60 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/useObserver.ts b/src/useObserver.ts index a9f711f1..b78111be 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -109,6 +109,16 @@ function cleanUncommittedReactions() { /** * Only to be used by test functions; do not export outside of mobx-react-lite */ +export 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) + reactionCleanupHandle = undefined + cleanUncommittedReactions() + } +} + export function resetCleanupScheduleForTests() { if (reactionCleanupHandle) { clearTimeout(reactionCleanupHandle) diff --git a/test/useObserver.test.tsx b/test/useObserver.test.tsx index d7dfc0f2..5d46bea5 100644 --- a/test/useObserver.test.tsx +++ b/test/useObserver.test.tsx @@ -5,7 +5,7 @@ import { act, cleanup, render } from "react-testing-library" import ReactDOM from "react-dom" import { useObserver } from "../src" -import { resetCleanupScheduleForTests } from "../src/useObserver" +import { forceCleanupTimerToRunNowForTests, resetCleanupScheduleForTests } from "../src/useObserver" afterEach(cleanup) @@ -110,3 +110,61 @@ test("uncommitted components should not leak observations", async () => { 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() + jest.useFakeTimers() + + 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}
) + + // We're going to render directly using ReactDOM, not react-testing-library, because we want + // to be able to get in and do nasty things before everything (including useEffects) have completed, + // and react-testing-library waits for all that, using act(). + + const rootNode = document.createElement("div") + + // N.B. We use raw ReactDOM.render here rather than react-testing-library because we want to + // get in before the full render+commit cycle has completed. + ReactDOM.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. + + + , + rootNode + ) + + // 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. + forceCleanupTimerToRunNowForTests() + + // Allow the useEffect calls to run to completion. + jest.runAllTimers() + act(() => { + // no-op + }) + + // count1 should still be being observed by Component1, + // but count2 should have had its reaction cleaned up. + expect(countIsObserved).toBeTruthy() +}) From f59a01d7585a7d9783802cdefbb7fc501cab88e4 Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Tue, 9 Apr 2019 11:33:11 +0100 Subject: [PATCH 06/11] Update test for missing changes to check Strict and non-Strict mode We had an existing test to check that observable changes between render and commit didn't go missing, but it only checked in Strict mode, and there's a problem with non-Strict mode. --- test/useObserver.test.tsx | 48 ++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/test/useObserver.test.tsx b/test/useObserver.test.tsx index 5d46bea5..0b155d3b 100644 --- a/test/useObserver.test.tsx +++ b/test/useObserver.test.tsx @@ -43,32 +43,38 @@ test("uncommitted observing components should not attempt state changes", () => } }) -test("observable changes before first commit are not lost", () => { - const store = mobx.observable({ value: "initial" }) +const strictModeValues = [true, false] +strictModeValues.forEach(strictMode => { + const modeName = strictMode ? "StrictMode" : "non-StrictMode" - const TestComponent = () => useObserver(() =>
{store.value}
) + test(`observable changes before first commit are not lost (${modeName})`, () => { + const store = mobx.observable({ value: "initial" }) - // Render our observing component wrapped in StrictMode, but using - // raw ReactDOM.render (not react-testing-library render) because we - // don't want the useEffect calls to have run just yet... - const rootNode = document.createElement("div") - ReactDOM.render( - - - , - rootNode - ) + const TestComponent = () => useObserver(() =>
{store.value}
) - // Change our observable. This is happening between the initial render of - // our component and its initial commit, so it isn't fully mounted yet. - // We want to ensure that the change isn't lost. - store.value = "changed" + // Render our observing component wrapped in StrictMode, but using + // raw ReactDOM.render (not react-testing-library render) because we + // don't want the useEffect calls to have run just yet... + const rootNode = document.createElement("div") - act(() => { - // no-op - }) + let elem = + if (strictMode) { + elem = {elem} + } + + ReactDOM.render(elem, rootNode) + + // Change our observable. This is happening between the initial render of + // our component and its initial commit, so it isn't fully mounted yet. + // We want to ensure that the change isn't lost. + store.value = "changed" - expect(rootNode.textContent).toBe("changed") + act(() => { + // no-op + }) + + expect(rootNode.textContent).toBe("changed") + }) }) test("uncommitted components should not leak observations", async () => { From 466fbfca500817de35e9709b7c15bbcb89fec583 Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Wed, 10 Apr 2019 20:31:17 +0100 Subject: [PATCH 07/11] Add cleanup tracking and more tests This adds full cleanup tracking, and even more tests: - we now track how long ago potentially leaked reactions were created, and only clean those that were leaked 'a while ago' - if a reaction is incorrectly disposed because a component went away for a very long time and came back again later (in a way React doesn't even do right now), we safely recreate it and re-render - trap the situation where a change is made to a tracked observable between first render and commit (where we couldn't force an update because we hadn't _been_ committed) and force a re-render - more unit tests --- src/printDebugValue.ts | 7 +- src/useObserver.ts | 177 ++++++++++++++---- test/printDebugValue.test.ts | 8 +- ...r.test.tsx => strictAndConcurrentMode.tsx} | 112 ++++++++++- 4 files changed, 245 insertions(+), 59 deletions(-) rename test/{useObserver.test.tsx => strictAndConcurrentMode.tsx} (58%) diff --git a/src/printDebugValue.ts b/src/printDebugValue.ts index ef0e4f83..8ef487fd 100644 --- a/src/printDebugValue.ts +++ b/src/printDebugValue.ts @@ -1,8 +1,5 @@ import { getDependencyTree, Reaction } from "mobx" -export function printDebugValue(v: React.MutableRefObject) { - if (!v.current) { - return "" - } - return getDependencyTree(v.current) +export function printDebugValue(v: Reaction) { + return getDependencyTree(v) } diff --git a/src/useObserver.ts b/src/useObserver.ts index b78111be..e3b1dd9e 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -12,6 +12,10 @@ export interface IUseObserverOptions { const EMPTY_OBJECT = {} +function observerComponentNameFor(baseComponentName: string) { + return `observer${baseComponentName}` +} + export function useObserver( fn: () => T, baseComponentName: string = "observed", @@ -24,27 +28,70 @@ export function useObserver( const wantedForceUpdateHook = options.useForceUpdate || useForceUpdate const forceUpdate = wantedForceUpdateHook() - const reaction = useRef(null) - const committed = useRef(false) + // StrictMode/ConcurrentMode/Suspense may mean that our component is + // rendered and abandoned multiple times, so we need to track leaked + // Reactions. + const reactionTrackingRef = useRef(null) + + if (!reactionTrackingRef.current) { + // First render for this component (or first time since a previous + // reaction from an abandoned render was disposed). + const trackingData: IReactionTracking = { + cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, + reaction: new Reaction(observerComponentNameFor(baseComponentName), () => { + // Observable has changed, meaning we want to re-render + // BUT if we're a component that hasn't yet got to the useEffect() + // stage, we might be a component that _started_ to render, but + // got dropped, and we don't want to make state changes then. + // (It triggers warnings in StrictMode, for a start.) + if (trackingData.mounted) { + // We have reached useEffect(), so we're mounted, and can trigger an update + forceUpdate() + } else { + // We haven't yet reached useEffect(), so we'll need to trigger a re-render + // when (and if) useEffect() arrives. The easiest way to do that is just to + // drop our current reaction and allow useEffect() to recreate it. + trackingData.reaction.dispose() + reactionTrackingRef.current = null + } + }) + } - if (!reaction.current) { - // First render for this component. Not yet committed. - reaction.current = new Reaction(`observer(${baseComponentName})`, () => { - // Observable has changed. Only force an update if we've definitely - // been committed. - if (committed.current) { - forceUpdate() - } - }) - scheduleCleanupOfReactionIfNotCommitted(reaction.current) + reactionTrackingRef.current = trackingData + scheduleCleanupOfReactionIfLeaked(reactionTrackingRef) } + const reaction = reactionTrackingRef.current!.reaction useDebugValue(reaction, printDebugValue) useEffect(() => { - recordReactionAsCommitted(reaction.current!) - committed.current = true - return () => reaction.current!.dispose() + // Called on first mount only + recordReactionAsCommitted(reactionTrackingRef) + + if (reactionTrackingRef.current) { + // Great. We've already got our reaction from our render; + // all we need to do is to record that it's now mounted, + // to allow future observable changes to trigger re-renders + reactionTrackingRef.current.mounted = true + } else { + // The reaction we set up in our render has been disposed. + // This is either due to bad timings of renderings, e.g. our + // component was paused for a _very_ long time, and our + // reaction got cleaned up, or we got a observable change + // between render and useEffect + + // Re-create the reaction + reactionTrackingRef.current = { + reaction: new Reaction(observerComponentNameFor(baseComponentName), () => { + // We've definitely already been mounted at this point + forceUpdate() + }), + cleanAt: Infinity + } + forceUpdate() + } + + return () => reactionTrackingRef.current!.reaction.dispose() }, []) // render the original component, but have the @@ -52,7 +99,7 @@ export function useObserver( // can be invalidated (see above) once a dependency changes let rendering!: T let exception - reaction.current.track(() => { + reaction.track(() => { try { rendering = fn() } catch (e) { @@ -60,39 +107,72 @@ export function useObserver( } }) if (exception) { - reaction.current.dispose() throw exception // re-throw any exceptions catched during rendering } return rendering } +// Reaction cleanup tracking. + +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 +} + /** - * Reactions created by components that have yet to be committed. + * 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. */ -const uncommittedReactions: Reaction[] = [] +export const CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS = 10_000 /** - * Latest 'uncommitted reactions' cleanup timer handle + * The frequency with which we'll check for leaked reactions. */ -let reactionCleanupHandle: number | undefined +export const CLEANUP_TIMER_LOOP_MILLIS = 10_000 -function scheduleCleanupOfReactionIfNotCommitted(reaction: Reaction) { - uncommittedReactions.push(reaction) - if (reactionCleanupHandle) { - window.clearTimeout(reactionCleanupHandle) +/** + * 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 + +function ensureCleanupTimerRunning() { + if (reactionCleanupHandle === undefined) { + reactionCleanupHandle = setTimeout(cleanUncommittedReactions, CLEANUP_TIMER_LOOP_MILLIS) } - // We currently have no cleanup timer running; schedule one - reactionCleanupHandle = window.setTimeout(cleanUncommittedReactions, 100) } -function recordReactionAsCommitted(reaction: Reaction) { - // It would be more efficient if we could use a Set instead of an Array, - // but mobx-react-lite currently supports MobX 4 and ES5, so we can't assume - // the presence of Set. - const i = uncommittedReactions.indexOf(reaction) - if (i) { - uncommittedReactions.splice(i, 1) - } +function scheduleCleanupOfReactionIfLeaked(ref: React.MutableRefObject) { + uncommittedReactionRefs.add(ref) + + ensureCleanupTimerRunning() +} + +function recordReactionAsCommitted(reactionRef: React.MutableRefObject) { + uncommittedReactionRefs.delete(reactionRef) } /** @@ -101,11 +181,30 @@ function recordReactionAsCommitted(reaction: Reaction) { function cleanUncommittedReactions() { reactionCleanupHandle = undefined - while (uncommittedReactions.length) { - uncommittedReactions.pop()!.dispose() + // Loop through all the candidate leaked reactions; those older + // than CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS get tidied. + + const now = Date.now() + for (const ref of uncommittedReactionRefs) { + const tracking = ref.current + if (tracking) { + if (now >= tracking.cleanAt) { + // It's time to tidy up this leaked reaction. + tracking.reaction.dispose() + ref.current = 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() } } +/* istanbul ignore next */ /** * Only to be used by test functions; do not export outside of mobx-react-lite */ @@ -114,15 +213,15 @@ export function forceCleanupTimerToRunNowForTests() { // to force it to run at awkward times in unit tests. if (reactionCleanupHandle) { clearTimeout(reactionCleanupHandle) - reactionCleanupHandle = undefined cleanUncommittedReactions() } } +/* istanbul ignore next */ export function resetCleanupScheduleForTests() { if (reactionCleanupHandle) { clearTimeout(reactionCleanupHandle) reactionCleanupHandle = undefined } - uncommittedReactions.length = 0 + uncommittedReactionRefs.clear() } diff --git a/test/printDebugValue.test.ts b/test/printDebugValue.test.ts index 85f3f86b..69754e51 100644 --- a/test/printDebugValue.test.ts +++ b/test/printDebugValue.test.ts @@ -17,13 +17,11 @@ test("printDebugValue", () => { } }) - const refLike = { - current: (disposer as any)[$mobx] - } + const value = (disposer as any)[$mobx] - expect(printDebugValue(refLike)).toMatchSnapshot() + expect(printDebugValue(value)).toMatchSnapshot() disposer() - expect(printDebugValue(refLike)).toMatchSnapshot() + expect(printDebugValue(value)).toMatchSnapshot() }) diff --git a/test/useObserver.test.tsx b/test/strictAndConcurrentMode.tsx similarity index 58% rename from test/useObserver.test.tsx rename to test/strictAndConcurrentMode.tsx index 0b155d3b..33da981f 100644 --- a/test/useObserver.test.tsx +++ b/test/strictAndConcurrentMode.tsx @@ -5,7 +5,12 @@ import { act, cleanup, render } from "react-testing-library" import ReactDOM from "react-dom" import { useObserver } from "../src" -import { forceCleanupTimerToRunNowForTests, resetCleanupScheduleForTests } from "../src/useObserver" +import { + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, + CLEANUP_TIMER_LOOP_MILLIS, + forceCleanupTimerToRunNowForTests, + resetCleanupScheduleForTests +} from "../src/useObserver" afterEach(cleanup) @@ -80,7 +85,11 @@ strictModeValues.forEach(strictMode => { 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 }) @@ -109,7 +118,9 @@ test("uncommitted components should not leak observations", async () => { ) // Allow any reaction-disposal cleanup timers to run - jest.runAllTimers() + 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. @@ -129,7 +140,12 @@ test("cleanup timer should not clean up recently-pended reactions", () => { // 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 }) @@ -145,9 +161,6 @@ test("cleanup timer should not clean up recently-pended reactions", () => { // and react-testing-library waits for all that, using act(). const rootNode = document.createElement("div") - - // N.B. We use raw ReactDOM.render here rather than react-testing-library because we want to - // get in before the full render+commit cycle has completed. ReactDOM.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 @@ -162,15 +175,94 @@ test("cleanup timer should not clean up recently-pended reactions", () => { // 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() - // Allow the useEffect calls to run to completion. - jest.runAllTimers() + // 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 + // no-op, but triggers effect flushing }) - // count1 should still be being observed by Component1, - // but count2 should have had its reaction cleaned up. + // count should still be observed + expect(countIsObserved).toBeTruthy() +}) + +test("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)) + + const TestComponent1 = () => useObserver(() =>
{store.count}
) + + // We're going to render directly using ReactDOM, not react-testing-library, because we want + // to be able to get in and do nasty things before everything (including useEffects) have completed, + // and react-testing-library waits for all that, using act(). + const rootNode = document.createElement("div") + ReactDOM.render( + + + , + rootNode + ) + + // 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(rootNode.textContent).toContain("42") }) From 7eb47d393d340223440734608386300b1f25949a Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Thu, 25 Apr 2019 20:11:59 +0100 Subject: [PATCH 08/11] Fix renamed test file When I renamed this file, I forgot the .test. suffix. D'oh. --- ...rictAndConcurrentMode.tsx => strictAndConcurrentMode.test.tsx} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{strictAndConcurrentMode.tsx => strictAndConcurrentMode.test.tsx} (100%) diff --git a/test/strictAndConcurrentMode.tsx b/test/strictAndConcurrentMode.test.tsx similarity index 100% rename from test/strictAndConcurrentMode.tsx rename to test/strictAndConcurrentMode.test.tsx From 502bb682223446c664f71510b379073c1d1fed61 Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Thu, 25 Apr 2019 20:28:52 +0100 Subject: [PATCH 09/11] Extract tracking and cleanup logic out to separate file --- src/reactionCleanupTracking.ts | 117 +++++++++++++++++++++++++ src/useObserver.ts | 120 ++------------------------ test/strictAndConcurrentMode.test.tsx | 2 +- 3 files changed, 124 insertions(+), 115 deletions(-) create mode 100644 src/reactionCleanupTracking.ts diff --git a/src/reactionCleanupTracking.ts b/src/reactionCleanupTracking.ts new file mode 100644 index 00000000..989ccd6f --- /dev/null +++ b/src/reactionCleanupTracking.ts @@ -0,0 +1,117 @@ +import { Reaction } from "mobx" + +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 +} + +/** + * 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 + +/** + * 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 + +function ensureCleanupTimerRunning() { + if (reactionCleanupHandle === undefined) { + reactionCleanupHandle = setTimeout(cleanUncommittedReactions, CLEANUP_TIMER_LOOP_MILLIS) + } +} + +export function scheduleCleanupOfReactionIfLeaked( + ref: React.MutableRefObject +) { + uncommittedReactionRefs.add(ref) + + ensureCleanupTimerRunning() +} + +export function recordReactionAsCommitted( + reactionRef: React.MutableRefObject +) { + 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() + for (const ref of uncommittedReactionRefs) { + const tracking = ref.current + if (tracking) { + if (now >= tracking.cleanAt) { + // It's time to tidy up this leaked reaction. + tracking.reaction.dispose() + ref.current = 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() + } +} + +/* istanbul ignore next */ +/** + * Only to be used by test functions; do not export outside of mobx-react-lite + */ +export 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 */ +export function resetCleanupScheduleForTests() { + if (reactionCleanupHandle) { + clearTimeout(reactionCleanupHandle) + reactionCleanupHandle = undefined + } + uncommittedReactionRefs.clear() +} diff --git a/src/useObserver.ts b/src/useObserver.ts index e3b1dd9e..317cefeb 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -1,6 +1,12 @@ import { Reaction } from "mobx" import { useDebugValue, useEffect, useRef } from "react" import { printDebugValue } from "./printDebugValue" +import { + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, + IReactionTracking, + recordReactionAsCommitted, + scheduleCleanupOfReactionIfLeaked +} from "./reactionCleanupTracking" import { isUsingStaticRendering } from "./staticRendering" import { useForceUpdate } from "./utils" @@ -111,117 +117,3 @@ export function useObserver( } return rendering } - -// Reaction cleanup tracking. - -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 -} - -/** - * 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 - -/** - * 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 - -function ensureCleanupTimerRunning() { - if (reactionCleanupHandle === undefined) { - reactionCleanupHandle = setTimeout(cleanUncommittedReactions, CLEANUP_TIMER_LOOP_MILLIS) - } -} - -function scheduleCleanupOfReactionIfLeaked(ref: React.MutableRefObject) { - uncommittedReactionRefs.add(ref) - - ensureCleanupTimerRunning() -} - -function recordReactionAsCommitted(reactionRef: React.MutableRefObject) { - 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() - for (const ref of uncommittedReactionRefs) { - const tracking = ref.current - if (tracking) { - if (now >= tracking.cleanAt) { - // It's time to tidy up this leaked reaction. - tracking.reaction.dispose() - ref.current = 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() - } -} - -/* istanbul ignore next */ -/** - * Only to be used by test functions; do not export outside of mobx-react-lite - */ -export 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 */ -export function resetCleanupScheduleForTests() { - if (reactionCleanupHandle) { - clearTimeout(reactionCleanupHandle) - reactionCleanupHandle = undefined - } - uncommittedReactionRefs.clear() -} diff --git a/test/strictAndConcurrentMode.test.tsx b/test/strictAndConcurrentMode.test.tsx index 33da981f..bfa5e56a 100644 --- a/test/strictAndConcurrentMode.test.tsx +++ b/test/strictAndConcurrentMode.test.tsx @@ -10,7 +10,7 @@ import { CLEANUP_TIMER_LOOP_MILLIS, forceCleanupTimerToRunNowForTests, resetCleanupScheduleForTests -} from "../src/useObserver" +} from "../src/reactionCleanupTracking" afterEach(cleanup) From c4c6bc6dd1e1d8a4893199d8fc75e2ec9741fc90 Mon Sep 17 00:00:00 2001 From: Daniel K Date: Sat, 27 Apr 2019 16:26:57 +0100 Subject: [PATCH 10/11] Update src/useObserver.ts Co-Authored-By: RoystonS --- src/useObserver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/useObserver.ts b/src/useObserver.ts index 317cefeb..c15d6d8f 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -67,7 +67,7 @@ export function useObserver( scheduleCleanupOfReactionIfLeaked(reactionTrackingRef) } - const reaction = reactionTrackingRef.current!.reaction + const { reaction } = reactionTrackingRef.current! useDebugValue(reaction, printDebugValue) useEffect(() => { From 6c78e5b25cc9f6e61c4372bb472aa488e12d8da4 Mon Sep 17 00:00:00 2001 From: Royston Shufflebotham Date: Sat, 27 Apr 2019 16:35:46 +0100 Subject: [PATCH 11/11] Move some more tracking internals into the tracking code --- src/reactionCleanupTracking.ts | 8 +++++++ src/useObserver.ts | 41 +++++++++++++++++----------------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/reactionCleanupTracking.ts b/src/reactionCleanupTracking.ts index 989ccd6f..3de158f8 100644 --- a/src/reactionCleanupTracking.ts +++ b/src/reactionCleanupTracking.ts @@ -22,6 +22,14 @@ export interface IReactionTracking { changedBeforeMount?: boolean } +export function createTrackingData(reaction: Reaction) { + const trackingData: IReactionTracking = { + cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, + reaction + } + return trackingData +} + /** * 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 diff --git a/src/useObserver.ts b/src/useObserver.ts index c15d6d8f..06383855 100644 --- a/src/useObserver.ts +++ b/src/useObserver.ts @@ -2,7 +2,7 @@ import { Reaction } from "mobx" import { useDebugValue, useEffect, useRef } from "react" import { printDebugValue } from "./printDebugValue" import { - CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, + createTrackingData, IReactionTracking, recordReactionAsCommitted, scheduleCleanupOfReactionIfLeaked @@ -42,27 +42,26 @@ export function useObserver( if (!reactionTrackingRef.current) { // First render for this component (or first time since a previous // reaction from an abandoned render was disposed). - const trackingData: IReactionTracking = { - cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, - reaction: new Reaction(observerComponentNameFor(baseComponentName), () => { - // Observable has changed, meaning we want to re-render - // BUT if we're a component that hasn't yet got to the useEffect() - // stage, we might be a component that _started_ to render, but - // got dropped, and we don't want to make state changes then. - // (It triggers warnings in StrictMode, for a start.) - if (trackingData.mounted) { - // We have reached useEffect(), so we're mounted, and can trigger an update - forceUpdate() - } else { - // We haven't yet reached useEffect(), so we'll need to trigger a re-render - // when (and if) useEffect() arrives. The easiest way to do that is just to - // drop our current reaction and allow useEffect() to recreate it. - trackingData.reaction.dispose() - reactionTrackingRef.current = null - } - }) - } + const newReaction = new Reaction(observerComponentNameFor(baseComponentName), () => { + // Observable has changed, meaning we want to re-render + // BUT if we're a component that hasn't yet got to the useEffect() + // stage, we might be a component that _started_ to render, but + // got dropped, and we don't want to make state changes then. + // (It triggers warnings in StrictMode, for a start.) + if (trackingData.mounted) { + // We have reached useEffect(), so we're mounted, and can trigger an update + forceUpdate() + } else { + // We haven't yet reached useEffect(), so we'll need to trigger a re-render + // when (and if) useEffect() arrives. The easiest way to do that is just to + // drop our current reaction and allow useEffect() to recreate it. + newReaction.dispose() + reactionTrackingRef.current = null + } + }) + + const trackingData = createTrackingData(newReaction) reactionTrackingRef.current = trackingData scheduleCleanupOfReactionIfLeaked(reactionTrackingRef) }