From 93b0dbd5f7ee54b3e3c3ba423e31b91abc99024d Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Wed, 21 Aug 2019 10:38:55 +0200 Subject: [PATCH] Memoize all the things. Fixes #80. --- .eslintrc | 1 + packages/react-async/src/useAsync.js | 139 +++++++++++++--------- packages/react-async/src/useAsync.spec.js | 34 +++++- 3 files changed, 119 insertions(+), 55 deletions(-) diff --git a/.eslintrc b/.eslintrc index 314b815f..dbd66a74 100644 --- a/.eslintrc +++ b/.eslintrc @@ -13,6 +13,7 @@ "plugins": ["jest", "promise", "react", "react-hooks"], "rules": { "react-hooks/rules-of-hooks": "error", + "react-hooks/exhaustive-deps": "warn", "no-console": "error" }, "settings": { diff --git a/packages/react-async/src/useAsync.js b/packages/react-async/src/useAsync.js index b2281aac..ffa0616f 100644 --- a/packages/react-async/src/useAsync.js +++ b/packages/react-async/src/useAsync.js @@ -21,49 +21,68 @@ const useAsync = (arg1, arg2) => { options, init ) - const dispatch = dispatcher - ? action => dispatcher(action, dispatchMiddleware(_dispatch), options) - : dispatchMiddleware(_dispatch) - - const getMeta = meta => ({ counter: counter.current, debugLabel: options.debugLabel, ...meta }) + const dispatch = useCallback( + dispatcher + ? action => dispatcher(action, dispatchMiddleware(_dispatch), lastOptions.current) + : dispatchMiddleware(_dispatch), + [dispatcher] + ) - const setData = (data, callback = noop) => { - if (isMounted.current) { - dispatch({ type: actionTypes.fulfill, payload: data, meta: getMeta() }) - callback() - } - return data - } + const { debugLabel } = options + const getMeta = useCallback(meta => ({ counter: counter.current, debugLabel, ...meta }), [ + debugLabel, + ]) + + const setData = useCallback( + (data, callback = noop) => { + if (isMounted.current) { + dispatch({ type: actionTypes.fulfill, payload: data, meta: getMeta() }) + callback() + } + return data + }, + [dispatch, getMeta] + ) - const setError = (error, callback = noop) => { - if (isMounted.current) { - dispatch({ type: actionTypes.reject, payload: error, error: true, meta: getMeta() }) - callback() - } - return error - } + const setError = useCallback( + (error, callback = noop) => { + if (isMounted.current) { + dispatch({ type: actionTypes.reject, payload: error, error: true, meta: getMeta() }) + callback() + } + return error + }, + [dispatch, getMeta] + ) const { onResolve, onReject } = options - const handleResolve = count => data => - count === counter.current && setData(data, () => onResolve && onResolve(data)) - const handleReject = count => error => - count === counter.current && setError(error, () => onReject && onReject(error)) - - const start = promiseFn => { - if ("AbortController" in globalScope) { - abortController.current.abort() - abortController.current = new globalScope.AbortController() - } - counter.current++ - return new Promise((resolve, reject) => { - if (!isMounted.current) return - const executor = () => promiseFn().then(resolve, reject) - dispatch({ type: actionTypes.start, payload: executor, meta: getMeta() }) - }) - } + const handleResolve = useCallback( + count => data => count === counter.current && setData(data, () => onResolve && onResolve(data)), + [setData, onResolve] + ) + const handleReject = useCallback( + count => err => count === counter.current && setError(err, () => onReject && onReject(err)), + [setError, onReject] + ) + + const start = useCallback( + promiseFn => { + if ("AbortController" in globalScope) { + abortController.current.abort() + abortController.current = new globalScope.AbortController() + } + counter.current++ + return new Promise((resolve, reject) => { + if (!isMounted.current) return + const executor = () => promiseFn().then(resolve, reject) + dispatch({ type: actionTypes.start, payload: executor, meta: getMeta() }) + }) + }, + [dispatch, getMeta] + ) const { promise, promiseFn, initialValue } = options - const load = () => { + const load = useCallback(() => { if (promise) { return start(() => promise).then( handleResolve(counter.current), @@ -77,26 +96,36 @@ const useAsync = (arg1, arg2) => { handleReject(counter.current) ) } - } + }, [start, promise, promiseFn, initialValue, handleResolve, handleReject]) const { deferFn } = options - const run = (...args) => { - if (deferFn) { - lastArgs.current = args - return start(() => deferFn(args, lastOptions.current, abortController.current)).then( - handleResolve(counter.current), - handleReject(counter.current) - ) - } - } + const run = useCallback( + (...args) => { + if (deferFn) { + lastArgs.current = args + return start(() => deferFn(args, lastOptions.current, abortController.current)).then( + handleResolve(counter.current), + handleReject(counter.current) + ) + } + }, + [start, deferFn, handleResolve, handleReject] + ) + + const reload = useCallback(() => { + return lastArgs.current ? run(...lastArgs.current) : load() + }, [run, load]) - const cancel = () => { - options.onCancel && options.onCancel() + const { onCancel } = options + const cancel = useCallback(() => { + onCancel && onCancel() counter.current++ abortController.current.abort() isMounted.current && dispatch({ type: actionTypes.cancel, meta: getMeta() }) - } + }, [onCancel, dispatch, getMeta]) + /* These effects should only be triggered on changes to specific props */ + /* eslint-disable react-hooks/exhaustive-deps */ const { watch, watchFn } = options useEffect(() => { if (watchFn && lastOptions.current && watchFn(options, lastOptions.current)) load() @@ -108,19 +137,20 @@ const useAsync = (arg1, arg2) => { }, [promise, promiseFn, watch]) useEffect(() => () => (isMounted.current = false), []) useEffect(() => () => cancel(), []) + /* eslint-enable react-hooks/exhaustive-deps */ useDebugValue(state, ({ status }) => `[${counter.current}] ${status}`) return useMemo( () => ({ ...state, - cancel, run, - reload: () => (lastArgs.current ? run(...lastArgs.current) : load()), + reload, + cancel, setData, setError, }), - [state, deferFn, onResolve, onReject, dispatcher, reducer] + [state, run, reload, cancel, setData, setError] ) } @@ -138,11 +168,12 @@ const useAsyncFetch = (input, init, { defer, json, ...options } = {}) => { const doFetch = (input, init) => globalScope.fetch(input, init).then(parseResponse(accept, json)) const isDefer = defer === true || ~["POST", "PUT", "PATCH", "DELETE"].indexOf(method) const fn = defer === false || !isDefer ? "promiseFn" : "deferFn" + const identity = JSON.stringify({ input, init }) const state = useAsync({ ...options, [fn]: useCallback( (_, props, ctrl) => doFetch(input, { signal: ctrl ? ctrl.signal : props.signal, ...init }), - [JSON.stringify(input), JSON.stringify(init)] + [identity] // eslint-disable-line react-hooks/exhaustive-deps ), }) useDebugValue(state, ({ counter, status }) => `[${counter}] ${status}`) diff --git a/packages/react-async/src/useAsync.spec.js b/packages/react-async/src/useAsync.spec.js index 54eefd9c..bc0cede5 100644 --- a/packages/react-async/src/useAsync.spec.js +++ b/packages/react-async/src/useAsync.spec.js @@ -54,7 +54,7 @@ describe("useAsync", () => { expect(onResolve).toHaveBeenCalledWith("done") }) - test("calling run() will always use the latest onResolve/onReject callbacks", async () => { + test("calling run() will always use the latest onResolve callback", async () => { const promiseFn = jest.fn(() => resolveTo()) const deferFn = () => resolveTo() function App() { @@ -84,6 +84,38 @@ describe("useAsync", () => { await sleep(10) // resolve deferFn expect(promiseFn).toHaveBeenLastCalledWith(expect.objectContaining({ count: 1 }), abortCtrl) }) + + test("calling run() will always use the latest onReject callback", async () => { + const onReject1 = jest.fn() + const onReject2 = jest.fn() + const deferFn = ([count]) => Promise.reject(count) + function App() { + const [count, setCount] = React.useState(0) + const onReject = count === 0 ? onReject1 : onReject2 + const { run } = useAsync({ deferFn, onReject }) + return + } + const { getByText } = render() + fireEvent.click(getByText("run")) + await sleep(10) // resolve deferFn + expect(onReject1).toHaveBeenCalledWith(0) + fireEvent.click(getByText("run")) + await sleep(10) // resolve deferFn + expect(onReject2).toHaveBeenCalledWith(1) + }) +}) + +test("does not return a new `run` function on every render", async () => { + const deferFn = () => resolveTo("done") + const DeleteScheduleForm = () => { + const [value, setValue] = React.useState() + const { run } = useAsync({ deferFn }) + React.useEffect(() => value && run() && undefined, [value, run]) + return + } + const component = + const { getByText } = render(component) + fireEvent.click(getByText("run")) }) describe("useFetch", () => {