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/reactionCleanupTracking.ts b/src/reactionCleanupTracking.ts new file mode 100644 index 00000000..3de158f8 --- /dev/null +++ b/src/reactionCleanupTracking.ts @@ -0,0 +1,125 @@ +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 +} + +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 + * 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 cfeaa648..06383855 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 { + createTrackingData, + IReactionTracking, + recordReactionAsCommitted, + scheduleCleanupOfReactionIfLeaked +} from "./reactionCleanupTracking" import { isUsingStaticRendering } from "./staticRendering" import { useForceUpdate } from "./utils" @@ -12,6 +18,10 @@ export interface IUseObserverOptions { const EMPTY_OBJECT = {} +function observerComponentNameFor(baseComponentName: string) { + return `observer${baseComponentName}` +} + export function useObserver( fn: () => T, baseComponentName: string = "observed", @@ -24,25 +34,69 @@ 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). - 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) { + 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) } + const { reaction } = reactionTrackingRef.current! useDebugValue(reaction, printDebugValue) useEffect(() => { - 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 @@ -50,7 +104,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) { @@ -58,7 +112,6 @@ export function useObserver( } }) if (exception) { - reaction.current.dispose() throw exception // re-throw any exceptions catched during rendering } return rendering 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/strictAndConcurrentMode.test.tsx b/test/strictAndConcurrentMode.test.tsx new file mode 100644 index 00000000..bfa5e56a --- /dev/null +++ b/test/strictAndConcurrentMode.test.tsx @@ -0,0 +1,268 @@ +import mockConsole from "jest-mock-console" +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 { + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, + CLEANUP_TIMER_LOOP_MILLIS, + forceCleanupTimerToRunNowForTests, + resetCleanupScheduleForTests +} from "../src/reactionCleanupTracking" + +afterEach(cleanup) + +test("uncommitted observing components should not attempt state changes", () => { + const store = mobx.observable({ count: 0 }) + + const TestComponent = () => useObserver(() =>
{store.count}
) + + // Render our observing component wrapped in StrictMode + const rendering = render( + + + + ) + + // That will have caused our component to have been rendered + // more than once, but when we unmount it'll only unmount once. + rendering.unmount() + + // Trigger a change to the observable. If the reactions were + // not disposed correctly, we'll see some console errors from + // React StrictMode because we're calling state mutators to + // trigger an update. + const restoreConsole = mockConsole() + try { + act(() => { + store.count++ + }) + + // Check to see if any console errors were reported. + // tslint:disable-next-line: no-console + expect(console.error).not.toHaveBeenCalled() + } finally { + restoreConsole() + } +}) + +const strictModeValues = [true, false] +strictModeValues.forEach(strictMode => { + const modeName = strictMode ? "StrictMode" : "non-StrictMode" + + test(`observable changes before first commit are not lost (${modeName})`, () => { + 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") + + 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" + + act(() => { + // no-op + }) + + expect(rootNode.textContent).toBe("changed") + }) +}) + +test("uncommitted components should not leak observations", async () => { + resetCleanupScheduleForTests() + + // Unfortunately, Jest fake timers don't mock out Date.now, so we fake + // that out in parallel to Jest useFakeTimers + let fakeNow = Date.now() + jest.useFakeTimers() + jest.spyOn(Date, "now").mockImplementation(() => fakeNow) + + const store = mobx.observable({ count1: 0, count2: 0 }) + + // Track whether counts are observed + let count1IsObserved = false + let count2IsObserved = false + mobx.onBecomeObserved(store, "count1", () => (count1IsObserved = true)) + mobx.onBecomeUnobserved(store, "count1", () => (count1IsObserved = false)) + mobx.onBecomeObserved(store, "count2", () => (count2IsObserved = true)) + mobx.onBecomeUnobserved(store, "count2", () => (count2IsObserved = false)) + + const TestComponent1 = () => useObserver(() =>
{store.count1}
) + const TestComponent2 = () => useObserver(() =>
{store.count2}
) + + // Render, then remove only #2 + const rendering = render( + + + + + ) + rendering.rerender( + + + + ) + + // Allow any reaction-disposal cleanup timers to run + const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS) + fakeNow += skip + jest.advanceTimersByTime(skip) + + // count1 should still be being observed by Component1, + // but count2 should have had its reaction cleaned up. + expect(count1IsObserved).toBeTruthy() + expect(count2IsObserved).toBeFalsy() +}) + +test("cleanup timer should not clean up recently-pended reactions", () => { + // If we're not careful with timings, it's possible to get the + // following scenario: + // 1. Component instance A is being created; it renders, we put its reaction R1 into the cleanup list + // 2. Strict/Concurrent mode causes that render to be thrown away + // 3. Component instance A is being created; it renders, we put its reaction R2 into the cleanup list + // 4. The MobX reaction timer from 5 seconds ago kicks in and cleans up all reactions from uncommitted + // components, including R1 and R2 + // 5. The commit phase runs for component A, but reaction R2 has already been disposed. Game over. + + // This unit test attempts to replicate that scenario: + resetCleanupScheduleForTests() + + // Unfortunately, Jest fake timers don't mock out Date.now, so we fake + // that out in parallel to Jest useFakeTimers + const fakeNow = Date.now() + jest.useFakeTimers() + jest.spyOn(Date, "now").mockImplementation(() => fakeNow) + + const store = mobx.observable({ count: 0 }) + + // Track whether the count is observed + let countIsObserved = false + mobx.onBecomeObserved(store, "count", () => (countIsObserved = true)) + mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false)) + + 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( + // 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. + + // We force our cleanup loop to run even though enough time hasn't _really_ + // elapsed. In theory, it won't do anything because not enough time has + // elapsed since the reactions were queued, and so they won't be disposed. + forceCleanupTimerToRunNowForTests() + + // Advance time enough to allow any timer-queued effects to run + jest.advanceTimersByTime(500) + + // Now allow the useEffect calls to run to completion. + act(() => { + // no-op, but triggers effect flushing + }) + + // count should still be observed + expect(countIsObserved).toBeTruthy() +}) + +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") +}) diff --git a/test/useObserver.test.tsx b/test/useObserver.test.tsx deleted file mode 100644 index 2e8be20c..00000000 --- a/test/useObserver.test.tsx +++ /dev/null @@ -1,42 +0,0 @@ -import mockConsole from "jest-mock-console" -import * as mobx from "mobx" -import * as React from "react" -import { act, cleanup, render } from "react-testing-library" - -import { useObserver } from "../src" - -afterEach(cleanup) - -test("uncommitted observing components should not attempt state changes", () => { - const store = mobx.observable({ count: 0 }) - - const TestComponent = () => useObserver(() =>
{store.count}
) - - // Render our observing component wrapped in StrictMode - const rendering = render( - - - - ) - - // That will have caused our component to have been rendered - // more than once, but when we unmount it'll only unmount once. - rendering.unmount() - - // Trigger a change to the observable. If the reactions were - // not disposed correctly, we'll see some console errors from - // React StrictMode because we're calling state mutators to - // trigger an update. - const restoreConsole = mockConsole() - try { - act(() => { - store.count++ - }) - - // Check to see if any console errors were reported. - // tslint:disable-next-line: no-console - expect(console.error).not.toHaveBeenCalled() - } finally { - restoreConsole() - } -})