fix for useDisposable, update some dev deps

README.md

@@ -7,6 +7,7 @@
 This is a next iteration of [mobx-react](https://github.com/mobxjs/mobx-react) coming from introducing React hooks which simplifies a lot of internal workings of this package. Class based components **are not supported** except using `<Observer>` directly in its `render` method.
 
 **You need React version 16.7.0-alpha.2 or 16.8.0-alpha.0 which is highly experimental and not recommended for production.**
+
 **Do not use React 16.7 as it's [missing Hooks support](https://reactjs.org/blog/2018/12/19/react-v-16-7.html)!**
 
 [![NPM](https://nodei.co/npm/mobx-react-lite.png)](https://www.npmjs.com/package/mobx-react-lite)
@@ -19,7 +20,7 @@ Project is written in TypeScript and provides type safety out of the box. No Flo
     -   [`useObserver<T>(fn: () => T, baseComponentName = "observed"): T`](#useobservertfn---t-basecomponentname--%22observed%22-t)
     -   [`useObservable<T>(initialValue: T): T`](#useobservabletinitialvalue-t-t)
     -   [`useComputed(func: () => T, inputs: ReadonlyArray<any> = []): T`](#usecomputedfunc---t-inputs-readonlyarrayany---t)
-    -   [`useDisposable<D extends IReactionDisposer>(disposerGenerator: () => D, inputs: ReadonlyArray<any> = []): D`](#usedisposabled-extends-ireactiondisposerdisposergenerator---d-inputs-readonlyarrayany---d)
+    -   [`useDisposable<D extends TDisposable>(disposerGenerator: () => D, inputs: ReadonlyArray<any> = []): D`](#usedisposabled-extends-ireactiondisposerdisposergenerator---d-inputs-readonlyarrayany---d)
 -   [Server Side Rendering with `useStaticRendering`](#server-side-rendering-with-usestaticrendering)
 -   [Why no Provider/inject?](#why-no-providerinject)
 -   [What about smart/dumb components?](#what-about-smartdumb-components)
@@ -203,7 +204,7 @@ Notice that since the computation depends on non-observable value, it has to be
 
 [![Edit Calculator](https://codesandbox.io/static/img/play-codesandbox.svg)](https://codesandbox.io/s/jzj48v2xry?module=%2Fsrc%2FCalculator.tsx)
 
-### `useDisposable<D extends IReactionDisposer>(disposerGenerator: () => D, inputs: ReadonlyArray<any> = []): D`
+### `useDisposable<D extends TDisposable>(disposerGenerator: () => D, inputs: ReadonlyArray<any> = []): D`
 
 The disposable is any kind of function that returns another function to be called on a component unmount to clean up used resources. Use MobX related functions like [`reaction`](https://mobx.js.org/refguide/reaction.html), [`autorun`](https://mobx.js.org/refguide/autorun.html), [`when`](https://mobx.js.org/refguide/when.html), [`observe`](https://mobx.js.org/refguide/observe.html), or anything else that returns a disposer.
 Returns the generated disposer for early disposal.
@@ -277,7 +278,7 @@ function App({ children }) {
 The React hooks don't force anyone to suddenly have a state inside a _dumb component_ that is supposed to only render stuff. You can separate your concerns in a similar fashion.
 
 ```tsx
-import { createSelector } from 'react-selector-hooks'
+import { createSelector } from "react-selector-hooks"
 
 const userSelector = createSelector(({ user }) => ({
     name: user.name,
@@ -297,9 +298,9 @@ export default () => {
     // you may extract these two lines into a custom hook
     const store = useContext(StoreContext)
     const data = userSelector(store)
-    return UiComponent({...data})
+    return UiComponent({ ...data })
     // perhaps wrap it inside observer in here?
-    return observer(UiComponent({...data}))
+    return observer(UiComponent({ ...data }))
 }
 ```
 
@@ -308,14 +309,13 @@ It may look a bit more verbose than a _classic_ inject, but there is nothing sto
 ```tsx
 // make universal HOC
 
-const inject = (useSelector, baseComponent) => (
-    React.useMemo((props) => {
+const inject = (useSelector, baseComponent) =>
+    React.useMemo(props => {
         const store = useContext(StoreContext)
         const selected = useSelector(store)
-        
+
         return baseComponent({ ...selected, ...props })
     })
-)
 
 // use the HOC with a selector
 

package.json

@@ -40,7 +40,7 @@
     "coveralls": "^3.0.2",
     "husky": "^1.1.3",
     "jest": "^23.6.0",
-    "jest-dom": "^2.1.1",
+    "jest-dom": "^3.0.0",
     "jest-environment-jsdom": "^23.4.0",
     "jest-mock-console": "^0.4.0",
     "lint-staged": "^8.0.4",
@@ -51,14 +51,14 @@
     "react-dom": "16.8.0-alpha.0",
     "react-testing-library": "^5.2.3",
     "rimraf": "^2.6.2",
-    "rollup": "^0.67.0",
+    "rollup": "^1.1.0",
     "rollup-plugin-alias": "^1.4.0",
     "rollup-plugin-commonjs": "^9.2.0",
-    "rollup-plugin-filesize": "^5.0.1",
-    "rollup-plugin-node-resolve": "^3.4.0",
+    "rollup-plugin-filesize": "^6.0.0",
+    "rollup-plugin-node-resolve": "^4.0.0",
     "rollup-plugin-replace": "^2.1.0",
-    "rollup-plugin-terser": "^3.0.0",
-    "rollup-plugin-typescript2": "^0.17.2",
+    "rollup-plugin-terser": "^4.0.0",
+    "rollup-plugin-typescript2": "^0.19.0",
     "ts-jest": "^23.10.4",
     "tslint": "^5.11.0",
     "tslint-config-prettier": "^1.15.0",

src/useDisposable.ts

@@ -1,7 +1,11 @@
-import { useEffect, useMemo, useRef } from "react"
+import { useLayoutEffect, useRef } from "react"
 
 type TDisposable = () => void
 
+const doNothingDisposer = () => {
+    // empty
+}
+
 /**
  * Adds an observable effect (reaction, autorun, or anything else that returns a disposer) that will be registered upon component creation and disposed upon unmounting.
  * Returns the generated disposer for early disposal.
@@ -17,19 +21,36 @@ export function useDisposable<D extends TDisposable>(
     inputs: ReadonlyArray<any> = []
 ): D {
     const disposerRef = useRef<D | undefined>(undefined)
+    const earlyDisposedRef = useRef(false)
 
-    useMemo(() => {
-        disposerRef.current = disposerGenerator()
+    // we need to use layout effect since disposals need to be run synchronously
+    useLayoutEffect(() => {
+        return lazyCreateDisposer(false)
     }, inputs)
 
-    useEffect(
-        () => () => {
-            if (disposerRef.current && typeof disposerRef.current === "function") {
+    function lazyCreateDisposer(earlyDisposal: boolean) {
+        // ensure that we won't create a new disposer if it was early disposed
+        if (earlyDisposedRef.current) {
+            return doNothingDisposer
+        }
+
+        if (!disposerRef.current) {
+            disposerRef.current = disposerGenerator()
+            if (typeof disposerRef.current !== "function") {
+                disposerRef.current = undefined
+                throw new Error("generated disposer must be a function")
+            }
+        }
+        return () => {
+            if (disposerRef.current) {
                 disposerRef.current()
+                disposerRef.current = undefined
+            }
+            if (earlyDisposal) {
+                earlyDisposedRef.current = true
             }
-        },
-        inputs
-    )
+        }
+    }
 
-    return disposerRef.current!
+    return lazyCreateDisposer(true) as D
 }

src/utils.ts

@@ -1,7 +1,7 @@
-import { useEffect } from "react"
+import { useLayoutEffect } from "react"
 
 const EMPTY_ARRAY: any[] = []
 
 export function useUnmount(fn: () => void) {
-    useEffect(() => fn, EMPTY_ARRAY)
+    useLayoutEffect(() => fn, EMPTY_ARRAY)
 }

test/useDisposable.test.tsx

@@ -3,103 +3,150 @@ import * as React from "react"
 import { cleanup, render } from "react-testing-library"
 
 import { observer, useDisposable } from "../src"
+import { withoutConsoleError } from "./utils"
 
 afterEach(cleanup)
 
 test("reactions run and dispose properly", async () => {
+    let reactions1Created = 0
+    let reactions2Created = 0
     let reactions1 = 0
     let reactions2 = 0
     let renders = 0
-    let reactionDisposerCalls = 0
+    let reaction1DisposerCalls = 0
+    let reaction2DisposerCalls = 0
 
     const store = observable({
         prop1: 0,
         prop2: 0
     })
 
-    const Component = observer((props: { store: typeof store }) => {
-        useDisposable(() => {
-            const disposer = reaction(
-                () => props.store.prop1,
-                () => {
-                    reactions1++
+    let firstReaction!: () => void
+
+    const Component = observer((props: { store: typeof store; a?: number }) => {
+        firstReaction = useDisposable(
+            () => {
+                reactions1Created++
+                const disposer = reaction(
+                    () => props.store.prop1,
+                    () => {
+                        reactions1++
+                    }
+                )
+
+                return () => {
+                    reaction1DisposerCalls++
+                    disposer()
                 }
-            )
-
-            return () => {
-                reactionDisposerCalls++
-                disposer()
-            }
-        })
+            },
+            [props.a]
+        )
 
-        useDisposable(() => {
-            const disposer = reaction(
-                () => props.store.prop2,
-                () => {
-                    reactions2++
+        useDisposable(
+            () => {
+                reactions2Created++
+                const disposer = reaction(
+                    () => props.store.prop2,
+                    () => {
+                        reactions2++
+                    }
+                )
+
+                return () => {
+                    reaction2DisposerCalls++
+                    disposer()
                 }
-            )
-
-            return () => {
-                reactionDisposerCalls++
-                disposer()
-            }
-        })
+            },
+            [props.a]
+        )
 
         renders++
         return (
             <div>
-                {props.store.prop1} {props.store.prop2}
+                {props.store.prop1} {props.store.prop2} {props.a}
             </div>
         )
     })
 
     const { rerender, unmount } = render(<Component store={store} />)
-    expect(reactionDisposerCalls).toBe(0)
+    expect(reactions1Created).toBe(1)
+    expect(reaction1DisposerCalls).toBe(0)
+    expect(reactions2Created).toBe(1)
+    expect(reaction2DisposerCalls).toBe(0)
     expect(renders).toBe(1)
     expect(reactions1).toBe(0)
     expect(reactions2).toBe(0)
 
     store.prop1 = 1
     rerender(<Component store={store} />)
-    expect(reactionDisposerCalls).toBe(0)
+    expect(reactions1Created).toBe(1)
+    expect(reaction1DisposerCalls).toBe(0)
+    expect(reactions2Created).toBe(1)
+    expect(reaction2DisposerCalls).toBe(0)
     expect(renders).toBe(2)
     expect(reactions1).toBe(1)
     expect(reactions2).toBe(0)
 
     store.prop2 = 1
     rerender(<Component store={store} />)
-    expect(reactionDisposerCalls).toBe(0)
+    expect(reactions1Created).toBe(1)
+    expect(reaction1DisposerCalls).toBe(0)
+    expect(reactions2Created).toBe(1)
+    expect(reaction2DisposerCalls).toBe(0)
     expect(renders).toBe(3)
     expect(reactions1).toBe(1)
     expect(reactions2).toBe(1)
 
+    // early dispose one of them, it shouldn't be re-created when one of the dependent inputs change
+    firstReaction()
+    expect(reactions1Created).toBe(1)
+    expect(reaction1DisposerCalls).toBe(1) // early disposal
+    expect(reactions2Created).toBe(1)
+    expect(reaction2DisposerCalls).toBe(0) // this one is not early disposed
+
+    rerender(<Component store={store} a={1} />)
+    expect(reactions1Created).toBe(1) // depends on a, but was early disposed, so it should not increment
+    expect(reaction1DisposerCalls).toBe(1)
+    expect(reactions2Created).toBe(2) // depends on a, so it gets re-created
+    expect(reaction2DisposerCalls).toBe(1)
+    expect(renders).toBe(4)
+    expect(reactions1).toBe(1)
+    expect(reactions2).toBe(1)
+
     unmount()
-    expect(reactionDisposerCalls).toBe(2)
-    expect(renders).toBe(3)
+    expect(reactions1Created).toBe(1)
+    expect(reaction1DisposerCalls).toBe(1)
+    expect(reactions2Created).toBe(2)
+    expect(reaction2DisposerCalls).toBe(2)
+    expect(renders).toBe(4)
     expect(reactions1).toBe(1)
     expect(reactions2).toBe(1)
 })
 
-test("disposer needs to be a function", async () => {
-    let renders = 0
+test("disposer needs to be a function or else throws", async () => {
+    const error = "generated disposer must be a function"
 
-    const Component = observer(() => {
+    const Component1 = observer(() => {
         useDisposable(() => {
             return undefined as any
         })
+        return <div>test</div>
+    })
 
+    const Component2 = observer(() => {
         useDisposable(() => {
-            return "I am not a disposer" as any
+            return "string" as any
         })
-
-        renders++
         return <div>test</div>
     })
 
-    const { unmount } = render(<Component />)
-    expect(renders).toBe(1)
+    await withoutConsoleError(async () => {
+        expect(() => {
+            render(<Component1 />)
+        }).toThrow(error)
 
-    unmount()
-    expect(renders).toBe(1)
+        expect(() => {
+            render(<Component2 />)
+        }).toThrow(error)
+    })
 })

test/useObservableEffect.test.tsx

@@ -2,6 +2,7 @@ import { IReactionDisposer, observable, reaction } from "mobx"
 import * as React from "react"
 import { cleanup, render } from "react-testing-library"
 import { observer, useObservableEffect } from "../src"
+import { withoutConsoleError } from "./utils"
 
 afterEach(cleanup)
 
@@ -80,21 +81,16 @@ test("reactions run and dispose properly", async () => {
     expect(reactions2).toBe(1)
 })
 
-test("an undefined disposer works", async () => {
-    let renders = 0
-
+test("an undefined disposer throws", async () => {
     const Component = observer(() => {
         useObservableEffect(() => {
             return undefined as any
         })
 
-        renders++
         return <div>test</div>
     })
 
-    const { rerender } = render(<Component />)
-    expect(renders).toBe(1)
-
-    rerender(<div />)
-    expect(renders).toBe(1)
+    await withoutConsoleError(async () => {
+        expect(() => render(<Component />)).toThrow("generated disposer must be a function")
+    })
 })