diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index 63d53b8508494..2dc1ddb02cac2 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -147,7 +147,7 @@ function useRef(initialValue: T): {current: T} { } function useLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, inputs: Array | void | null, ): void { nextHook(); @@ -159,7 +159,7 @@ function useLayoutEffect( } function useEffect( - create: () => mixed, + create: () => (() => void) | void, inputs: Array | void | null, ): void { nextHook(); diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 82c61626ef088..b9ec4e118a733 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -375,8 +375,8 @@ function useRef(initialValue: T): {current: T} { } export function useLayoutEffect( - create: () => mixed, - deps: Array | void | null, + create: () => (() => void) | void, + inputs: Array | void | null, ) { if (__DEV__) { currentHookNameInDev = 'useLayoutEffect'; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 76e136dd7dbf5..1ffb2f0eff4a0 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -320,42 +320,49 @@ function commitHookEffectList( if ((effect.tag & unmountTag) !== NoHookEffect) { // Unmount const destroy = effect.destroy; - effect.destroy = null; - if (destroy !== null) { + effect.destroy = undefined; + if (destroy !== undefined) { destroy(); } } if ((effect.tag & mountTag) !== NoHookEffect) { // Mount const create = effect.create; - let destroy = create(); - if (typeof destroy !== 'function') { - if (__DEV__) { - if (destroy !== null && destroy !== undefined) { - warningWithoutStack( - false, - 'useEffect function must return a cleanup function or ' + - 'nothing.%s%s', - typeof destroy.then === 'function' - ? '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' + - 'Instead, you may write an async function separately ' + - 'and then call it from inside the effect:\n\n' + - 'async function fetchComment(commentId) {\n' + - ' // You can await here\n' + - '}\n\n' + - 'useEffect(() => {\n' + - ' fetchComment(commentId);\n' + - '}, [commentId]);\n\n' + - 'In the future, React will provide a more idiomatic solution for data fetching ' + - "that doesn't involve writing effects manually." - : '', - getStackByFiberInDevAndProd(finishedWork), - ); + effect.destroy = create(); + + if (__DEV__) { + const destroy = effect.destroy; + if (destroy !== undefined && typeof destroy !== 'function') { + let addendum; + if (destroy === null) { + addendum = + ' You returned null. If your effect does not require clean ' + + 'up, return undefined (or nothing).'; + } else if (typeof destroy.then === 'function') { + addendum = + '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' + + 'Instead, you may write an async function separately ' + + 'and then call it from inside the effect:\n\n' + + 'async function fetchComment(commentId) {\n' + + ' // You can await here\n' + + '}\n\n' + + 'useEffect(() => {\n' + + ' fetchComment(commentId);\n' + + '}, [commentId]);\n\n' + + 'In the future, React will provide a more idiomatic solution for data fetching ' + + "that doesn't involve writing effects manually."; + } else { + addendum = ' You returned: ' + destroy; } + warningWithoutStack( + false, + 'An Effect function must not return anything besides a function, ' + + 'which is used for clean-up.%s%s', + addendum, + getStackByFiberInDevAndProd(finishedWork), + ); } - destroy = null; } - effect.destroy = destroy; } effect = effect.next; } while (effect !== firstEffect); @@ -696,7 +703,7 @@ function commitUnmount(current: Fiber): void { let effect = firstEffect; do { const destroy = effect.destroy; - if (destroy !== null) { + if (destroy !== undefined) { safelyCallDestroy(current, destroy); } effect = effect.next; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 94f61e03dacf2..e05a15d727742 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -59,8 +59,14 @@ export type Dispatcher = { observedBits: void | number | boolean, ): T, useRef(initialValue: T): {current: T}, - useEffect(create: () => mixed, deps: Array | void | null): void, - useLayoutEffect(create: () => mixed, deps: Array | void | null): void, + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void, + useLayoutEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void, useCallback(callback: T, deps: Array | void | null): T, useMemo(nextCreate: () => T, deps: Array | void | null): T, useImperativeHandle( @@ -119,8 +125,8 @@ type HookDev = Hook & { type Effect = { tag: HookEffectTag, - create: () => mixed, - destroy: (() => mixed) | null, + create: () => (() => void) | void, + destroy: (() => void) | void, deps: Array | null, next: Effect, }; @@ -780,13 +786,13 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void { const hook = mountWorkInProgressHook(); const nextDeps = deps === undefined ? null : deps; sideEffectTag |= fiberEffectTag; - hook.memoizedState = pushEffect(hookEffectTag, create, null, nextDeps); + hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps); } function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void { const hook = updateWorkInProgressHook(); const nextDeps = deps === undefined ? null : deps; - let destroy = null; + let destroy = undefined; if (currentHook !== null) { const prevEffect = currentHook.memoizedState; @@ -805,7 +811,7 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void { } function mountEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { return mountEffectImpl( @@ -817,7 +823,7 @@ function mountEffect( } function updateEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { return updateEffectImpl( @@ -829,7 +835,7 @@ function updateEffect( } function mountLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { return mountEffectImpl( @@ -841,7 +847,7 @@ function mountLayoutEffect( } function updateLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { return updateEffectImpl( @@ -860,7 +866,9 @@ function imperativeHandleEffect( const refCallback = ref; const inst = create(); refCallback(inst); - return () => refCallback(null); + return () => { + refCallback(null); + }; } else if (ref !== null && ref !== undefined) { const refObject = ref; if (__DEV__) { @@ -1205,7 +1213,10 @@ if (__DEV__) { currentHookNameInDev = 'useContext'; return mountContext(context, observedBits); }, - useEffect(create: () => mixed, deps: Array | void | null): void { + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { currentHookNameInDev = 'useEffect'; return mountEffect(create, deps); }, @@ -1218,7 +1229,7 @@ if (__DEV__) { return mountImperativeHandle(ref, create, deps); }, useLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { currentHookNameInDev = 'useLayoutEffect'; @@ -1289,7 +1300,10 @@ if (__DEV__) { currentHookNameInDev = 'useContext'; return updateContext(context, observedBits); }, - useEffect(create: () => mixed, deps: Array | void | null): void { + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { currentHookNameInDev = 'useEffect'; return updateEffect(create, deps); }, @@ -1302,7 +1316,7 @@ if (__DEV__) { return updateImperativeHandle(ref, create, deps); }, useLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { currentHookNameInDev = 'useLayoutEffect'; @@ -1376,7 +1390,10 @@ if (__DEV__) { warnInvalidHookAccess(); return mountContext(context, observedBits); }, - useEffect(create: () => mixed, deps: Array | void | null): void { + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); return mountEffect(create, deps); @@ -1391,7 +1408,7 @@ if (__DEV__) { return mountImperativeHandle(ref, create, deps); }, useLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { currentHookNameInDev = 'useLayoutEffect'; @@ -1471,7 +1488,10 @@ if (__DEV__) { warnInvalidHookAccess(); return updateContext(context, observedBits); }, - useEffect(create: () => mixed, deps: Array | void | null): void { + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); return updateEffect(create, deps); @@ -1486,7 +1506,7 @@ if (__DEV__) { return updateImperativeHandle(ref, create, deps); }, useLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, deps: Array | void | null, ): void { currentHookNameInDev = 'useLayoutEffect'; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 589f5c67e88aa..43010aac5e62b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -542,30 +542,40 @@ describe('ReactHooks', () => { ]); }); - it('warns for bad useEffect return values', () => { + it('assumes useEffect clean-up function is either a function or undefined', () => { const {useLayoutEffect} = React; + function App(props) { useLayoutEffect(() => { return props.return; }); return null; } - let root; - expect(() => { - root = ReactTestRenderer.create(); - }).toWarnDev([ - 'Warning: useEffect function must return a cleanup function or ' + - 'nothing.\n' + - ' in App (at **)', + const root1 = ReactTestRenderer.create(null); + expect(() => root1.update()).toWarnDev([ + 'Warning: An Effect function must not return anything besides a ' + + 'function, which is used for clean-up. You returned: 17', ]); - expect(() => { - root.update(); - }).toWarnDev([ - 'Warning: useEffect function must return a cleanup function or nothing.\n\n' + + const root2 = ReactTestRenderer.create(null); + expect(() => root2.update()).toWarnDev([ + 'Warning: An Effect function must not return anything besides a ' + + 'function, which is used for clean-up. You returned null. If your ' + + 'effect does not require clean up, return undefined (or nothing).', + ]); + + const root3 = ReactTestRenderer.create(null); + expect(() => root3.update()).toWarnDev([ + 'Warning: An Effect function must not return anything besides a ' + + 'function, which is used for clean-up.\n\n' + 'It looks like you wrote useEffect(async () => ...) or returned a Promise.', ]); + + // Error on unmount because React assumes the value is a function + expect(() => { + root3.update(null); + }).toThrow('is not a function'); }); it('warns for bad useImperativeHandle first arg', () => { diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index b7ba16ba5dd3a..b0debda90e346 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -84,7 +84,7 @@ export function useRef(initialValue: T): {current: T} { } export function useEffect( - create: () => mixed, + create: () => (() => void) | void, inputs: Array | void | null, ) { const dispatcher = resolveDispatcher(); @@ -92,7 +92,7 @@ export function useEffect( } export function useLayoutEffect( - create: () => mixed, + create: () => (() => void) | void, inputs: Array | void | null, ) { const dispatcher = resolveDispatcher();