Skip to content

Commit 8b34843

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 8b34843

2 files changed

Lines changed: 42 additions & 2 deletions

File tree

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,21 @@ 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__ && destroy !== null && destroy !== undefined) {
301+
warningWithoutStack(
302+
false,
303+
'useEffect function must return a cleanup function or nothing.%s%s',
304+
typeof destroy.then === 'function'
305+
? ' Promises and async functions are not supported.'
306+
: '',
307+
getStackByFiberInDevAndProd(finishedWork),
308+
);
309+
}
310+
destroy = null;
311+
}
312+
effect.destroy = destroy;
300313
}
301314
effect = effect.next;
302315
} while (effect !== firstEffect);

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,31 @@ 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 async functions are not supported.\n' +
78+
' in App (at **)',
79+
]);
80+
});
5481
});

0 commit comments

Comments
 (0)