Make StrictMode happy (#119)

* Add test for proper reaction disposal in StrictMode

* Use jest-mock-console instead of mocking console.error by hand

* Move test into separate useObserver test file

I've tweaked the test name to reflect what it's really checking for.

* Add a second test to verify that reactions are disposed after unmount

N.B. Depending on the implementation of a fix, this test might need some
extra delays or act() calls to trigger a delayed cleanup before asserting that
all is clean.

* Remove 'leak' test as it's a separately-solvable issue.

(I'll move this test into another PR.)

* Track whether the current component has been committed before forcing update

* Remove the now-unused useUnmount

src/useObserver.ts

@@ -1,8 +1,8 @@
 import { Reaction } from "mobx"
-import { useDebugValue, useRef } from "react"
+import { useDebugValue, useEffect, useRef } from "react"
 import { printDebugValue } from "./printDebugValue"
 import { isUsingStaticRendering } from "./staticRendering"
-import { useForceUpdate, useUnmount } from "./utils"
+import { useForceUpdate } from "./utils"
 
 export type ForceUpdateHook = () => () => void
 
@@ -25,17 +25,25 @@ export function useObserver<T>(
     const forceUpdate = wantedForceUpdateHook()
 
     const reaction = useRef<Reaction | null>(null)
+    const committed = useRef(false)
+
     if (!reaction.current) {
+        // First render for this component. Not yet committed.
         reaction.current = new Reaction(`observer(${baseComponentName})`, () => {
-            forceUpdate()
+            // Observable has changed. Only force an update if we've definitely
+            // been committed.
+            if (committed.current) {
+                forceUpdate()
+            }
         })
     }
 
     useDebugValue(reaction, printDebugValue)
 
-    useUnmount(() => {
-        reaction.current!.dispose()
-    })
+    useEffect(() => {
+        committed.current = true
+        return () => reaction.current!.dispose()
+    }, [])
 
     // render the original component, but have the
     // reaction track the observables, so that rendering

src/utils.ts

@@ -1,10 +1,4 @@
-import { useCallback, useEffect, useState } from "react"
-
-const EMPTY_ARRAY: any[] = []
-
-export function useUnmount(fn: () => void) {
-    useEffect(() => fn, EMPTY_ARRAY)
-}
+import { useCallback, useState } from "react"
 
 export function useForceUpdate() {
     const [, setTick] = useState(0)

test/useObserver.test.tsx

@@ -0,0 +1,42 @@
+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(() => <div>{store.count}</div>)
+
+    // Render our observing component wrapped in StrictMode
+    const rendering = render(
+        <React.StrictMode>
+            <TestComponent />
+        </React.StrictMode>
+    )
+
+    // 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()
+    }
+})