Warn for bad useEffect return value (#14069)
Mostly to catch this: ```js useEffect(async () => { // ... return cleanup; }); ``` 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.
Sophie Alpert committed
Nov 2, 2018 at 14:43 UTC
293fed89937201b780f0f567ae4d5dc01160dc55
2 files changed
+48
-2
packages/react-reconciler/src/ReactFiberCommitWork.js
+20
-2
@@ -295,8 +295,26 @@ function commitHookEffectList(
295
if ((effect.tag & mountTag) !== NoHookEffect) {
296
// Mount
297
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;
318
}
319
effect = effect.next;
320
} while (effect !== firstEffect);
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
+28
@@ -51,4 +51,32 @@ describe('ReactHooks', () => {
51
]);
52
expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
53
});
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
+ });
82
});