Skip to content

Commit 3f368be

Browse files
committed
Warn for bad useEffect return value
Is this too restrictive? Not sure if you would want to do like ```js useEffect(() => ref.current.style.color = 'red'); ``` which would give a false positive here. We can always relax it to only warn on Promises if people complain.
1 parent 595b4f9 commit 3f368be

2 files changed

Lines changed: 48 additions & 2 deletions

File tree

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,26 @@ function commitHookEffectList(
295295
if ((effect.tag & mountTag) !== NoHookEffect) {
296296
// Mount
297297
const create = effect.create;
298-
const destroy = create();
299-
effect.destroy = typeof destroy === 'function' ? destroy : null;
298+
let destroy = create();
299+
if (typeof destroy !== 'function') {
300+
if (__DEV__) {
301+
if (destroy !== null && destroy !== undefined) {
302+
warningWithoutStack(
303+
false,
304+
'useEffect function must return a cleanup function or ' +
305+
'nothing.%s%s',
306+
typeof destroy.then === 'function'
307+
? ' Promises and useEffect(async () => ...) are not ' +
308+
'supported, but you can call an async function inside an ' +
309+
'effect.'
310+
: '',
311+
getStackByFiberInDevAndProd(finishedWork),
312+
);
313+
}
314+
}
315+
destroy = null;
316+
}
317+
effect.destroy = destroy;
300318
}
301319
effect = effect.next;
302320
} while (effect !== firstEffect);

packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,32 @@ describe('ReactHooks', () => {
5151
]);
5252
expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
5353
});
54+
55+
it('warns for bad useEffect return values', () => {
56+
const {useLayoutEffect} = React;
57+
function App(props) {
58+
useLayoutEffect(() => {
59+
return props.return;
60+
});
61+
return null;
62+
}
63+
let root;
64+
65+
expect(() => {
66+
root = ReactTestRenderer.create(<App return={17} />);
67+
}).toWarnDev([
68+
'Warning: useEffect function must return a cleanup function or ' +
69+
'nothing.\n' +
70+
' in App (at **)',
71+
]);
72+
73+
expect(() => {
74+
root.update(<App return={Promise.resolve()} />);
75+
}).toWarnDev([
76+
'Warning: useEffect function must return a cleanup function or ' +
77+
'nothing. Promises and useEffect(async () => ...) are not supported, ' +
78+
'but you can call an async function inside an effect.\n' +
79+
' in App (at **)',
80+
]);
81+
});
5482
});

0 commit comments

Comments
 (0)