From ef7416ba89fa8441754358d0cd179b902b723400 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Thu, 9 Sep 2021 11:59:59 -0400 Subject: [PATCH 1/3] update error message to include useLayoutEffect or useEffect on bad effect return --- .../src/ReactFiberCommitWork.new.js | 19 +++++++++++++++---- .../src/ReactFiberCommitWork.old.js | 19 +++++++++++++++---- .../ReactHooksWithNoopRenderer-test.js | 14 +++++++------- 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index b38b888fd4029..dee8220d48b89 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -38,6 +38,7 @@ import { enableSuspenseLayoutEffectSemantics, enableUpdaterTracking, } from 'shared/ReactFeatureFlags'; +import {Layout, HasEffect} from './ReactHookEffectTags'; import { FunctionComponent, ForwardRef, @@ -507,7 +508,7 @@ function commitHookEffectListUnmount( } } -function commitHookEffectListMount(tag: number, finishedWork: Fiber) { +function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -522,6 +523,12 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { if (__DEV__) { const destroy = effect.destroy; if (destroy !== undefined && typeof destroy !== 'function') { + let hookName; + if (effect.tag === (HasEffect | Layout)) { + hookName = 'useLayoutEffect'; + } else { + hookName = 'useEffect'; + } let addendum; if (destroy === null) { addendum = @@ -529,10 +536,13 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { 'up, return undefined (or nothing).'; } else if (typeof destroy.then === 'function') { addendum = - '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' + + '\n\nIt looks like you wrote ' + + hookName + + '(async () => ...) or returned a Promise. ' + 'Instead, write the async function inside your effect ' + 'and call it immediately:\n\n' + - 'useEffect(() => {\n' + + hookName + + '(() => {\n' + ' async function fetchData() {\n' + ' // You can await here\n' + ' const response = await MyAPI.getData(someId);\n' + @@ -545,8 +555,9 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { addendum = ' You returned: ' + destroy; } console.error( - 'An effect function must not return anything besides a function, ' + + '%s must not return anything besides a function, ' + 'which is used for clean-up.%s', + hookName, addendum, ); } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 2bae266c60dc2..51c6530a41f35 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -38,6 +38,7 @@ import { enableSuspenseLayoutEffectSemantics, enableUpdaterTracking, } from 'shared/ReactFeatureFlags'; +import {Layout, HasEffect} from './ReactHookEffectTags'; import { FunctionComponent, ForwardRef, @@ -507,7 +508,7 @@ function commitHookEffectListUnmount( } } -function commitHookEffectListMount(tag: number, finishedWork: Fiber) { +function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -522,6 +523,12 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { if (__DEV__) { const destroy = effect.destroy; if (destroy !== undefined && typeof destroy !== 'function') { + let hookName; + if (effect.tag === (HasEffect | Layout)) { + hookName = 'useLayoutEffect'; + } else { + hookName = 'useEffect'; + } let addendum; if (destroy === null) { addendum = @@ -529,10 +536,13 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { 'up, return undefined (or nothing).'; } else if (typeof destroy.then === 'function') { addendum = - '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' + + '\n\nIt looks like you wrote ' + + hookName + + '(async () => ...) or returned a Promise. ' + 'Instead, write the async function inside your effect ' + 'and call it immediately:\n\n' + - 'useEffect(() => {\n' + + hookName + + '(() => {\n' + ' async function fetchData() {\n' + ' // You can await here\n' + ' const response = await MyAPI.getData(someId);\n' + @@ -545,8 +555,9 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { addendum = ' You returned: ' + destroy; } console.error( - 'An effect function must not return anything besides a function, ' + + '%s must not return anything besides a function, ' + 'which is used for clean-up.%s', + hookName, addendum, ); } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 342c8abf9a254..9069ca71c71e4 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2647,7 +2647,7 @@ describe('ReactHooksWithNoopRenderer', () => { root1.render(); }), ).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + + 'Warning: useEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); @@ -2657,7 +2657,7 @@ describe('ReactHooksWithNoopRenderer', () => { root2.render(); }), ).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + + 'Warning: useEffect 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).', ]); @@ -2668,7 +2668,7 @@ describe('ReactHooksWithNoopRenderer', () => { root3.render(); }), ).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + + 'Warning: useEffect 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.', ]); @@ -2873,7 +2873,7 @@ describe('ReactHooksWithNoopRenderer', () => { root1.render(); }), ).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + + 'Warning: useLayoutEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); @@ -2883,7 +2883,7 @@ describe('ReactHooksWithNoopRenderer', () => { root2.render(); }), ).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + + 'Warning: useLayoutEffect 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).', ]); @@ -2894,9 +2894,9 @@ describe('ReactHooksWithNoopRenderer', () => { root3.render(); }), ).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + + 'Warning: useLayoutEffect 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.', + 'It looks like you wrote useLayoutEffect(async () => ...) or returned a Promise.', ]); // Error on unmount because React assumes the value is a function From 527d01f356207f1ec001129952c27231371c8da6 Mon Sep 17 00:00:00 2001 From: salazarm Date: Thu, 9 Sep 2021 16:20:09 -0400 Subject: [PATCH 2/3] Update packages/react-reconciler/src/ReactFiberCommitWork.new.js Co-authored-by: Ricky --- packages/react-reconciler/src/ReactFiberCommitWork.new.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index dee8220d48b89..045b71b1ae11f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -524,7 +524,7 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { const destroy = effect.destroy; if (destroy !== undefined && typeof destroy !== 'function') { let hookName; - if (effect.tag === (HasEffect | Layout)) { + if ((effect.tag & Layout) !== NoFlags) { hookName = 'useLayoutEffect'; } else { hookName = 'useEffect'; From 5e010af95260c1ba99aa5de5803e3d7ef6312e43 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Thu, 9 Sep 2021 16:23:22 -0400 Subject: [PATCH 3/3] use existing import --- packages/react-reconciler/src/ReactFiberCommitWork.new.js | 3 +-- packages/react-reconciler/src/ReactFiberCommitWork.old.js | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 045b71b1ae11f..bde9173a28f69 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -38,7 +38,6 @@ import { enableSuspenseLayoutEffectSemantics, enableUpdaterTracking, } from 'shared/ReactFeatureFlags'; -import {Layout, HasEffect} from './ReactHookEffectTags'; import { FunctionComponent, ForwardRef, @@ -524,7 +523,7 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { const destroy = effect.destroy; if (destroy !== undefined && typeof destroy !== 'function') { let hookName; - if ((effect.tag & Layout) !== NoFlags) { + if ((effect.tag & HookLayout) !== NoFlags) { hookName = 'useLayoutEffect'; } else { hookName = 'useEffect'; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 51c6530a41f35..241bc5a5dbae3 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -38,7 +38,6 @@ import { enableSuspenseLayoutEffectSemantics, enableUpdaterTracking, } from 'shared/ReactFeatureFlags'; -import {Layout, HasEffect} from './ReactHookEffectTags'; import { FunctionComponent, ForwardRef, @@ -524,7 +523,7 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { const destroy = effect.destroy; if (destroy !== undefined && typeof destroy !== 'function') { let hookName; - if (effect.tag === (HasEffect | Layout)) { + if ((effect.tag & HookLayout) !== NoFlags) { hookName = 'useLayoutEffect'; } else { hookName = 'useEffect';