Skip to content

Commit 66eb293

Browse files
authored
Restrict effect return type to a function or nothing (#14119)
* Restrict effect return type to a function or nothing We already warn in dev if the wrong type is returned. This updates the Flow type. * Restrict return type further * Assume Effect hook returns either a function or undefined * Tweak warning message
1 parent 51c0791 commit 66eb293

File tree

6 files changed

+102
-65
lines changed

6 files changed

+102
-65
lines changed

packages/react-debug-tools/src/ReactDebugHooks.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ function useRef<T>(initialValue: T): {current: T} {
147147
}
148148

149149
function useLayoutEffect(
150-
create: () => mixed,
150+
create: () => (() => void) | void,
151151
inputs: Array<mixed> | void | null,
152152
): void {
153153
nextHook();
@@ -159,7 +159,7 @@ function useLayoutEffect(
159159
}
160160

161161
function useEffect(
162-
create: () => mixed,
162+
create: () => (() => void) | void,
163163
inputs: Array<mixed> | void | null,
164164
): void {
165165
nextHook();

packages/react-dom/src/server/ReactPartialRendererHooks.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -375,8 +375,8 @@ function useRef<T>(initialValue: T): {current: T} {
375375
}
376376

377377
export function useLayoutEffect(
378-
create: () => mixed,
379-
deps: Array<mixed> | void | null,
378+
create: () => (() => void) | void,
379+
inputs: Array<mixed> | void | null,
380380
) {
381381
if (__DEV__) {
382382
currentHookNameInDev = 'useLayoutEffect';

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -320,42 +320,49 @@ function commitHookEffectList(
320320
if ((effect.tag & unmountTag) !== NoHookEffect) {
321321
// Unmount
322322
const destroy = effect.destroy;
323-
effect.destroy = null;
324-
if (destroy !== null) {
323+
effect.destroy = undefined;
324+
if (destroy !== undefined) {
325325
destroy();
326326
}
327327
}
328328
if ((effect.tag & mountTag) !== NoHookEffect) {
329329
// Mount
330330
const create = effect.create;
331-
let destroy = create();
332-
if (typeof destroy !== 'function') {
333-
if (__DEV__) {
334-
if (destroy !== null && destroy !== undefined) {
335-
warningWithoutStack(
336-
false,
337-
'useEffect function must return a cleanup function or ' +
338-
'nothing.%s%s',
339-
typeof destroy.then === 'function'
340-
? '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' +
341-
'Instead, you may write an async function separately ' +
342-
'and then call it from inside the effect:\n\n' +
343-
'async function fetchComment(commentId) {\n' +
344-
' // You can await here\n' +
345-
'}\n\n' +
346-
'useEffect(() => {\n' +
347-
' fetchComment(commentId);\n' +
348-
'}, [commentId]);\n\n' +
349-
'In the future, React will provide a more idiomatic solution for data fetching ' +
350-
"that doesn't involve writing effects manually."
351-
: '',
352-
getStackByFiberInDevAndProd(finishedWork),
353-
);
331+
effect.destroy = create();
332+
333+
if (__DEV__) {
334+
const destroy = effect.destroy;
335+
if (destroy !== undefined && typeof destroy !== 'function') {
336+
let addendum;
337+
if (destroy === null) {
338+
addendum =
339+
' You returned null. If your effect does not require clean ' +
340+
'up, return undefined (or nothing).';
341+
} else if (typeof destroy.then === 'function') {
342+
addendum =
343+
'\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' +
344+
'Instead, you may write an async function separately ' +
345+
'and then call it from inside the effect:\n\n' +
346+
'async function fetchComment(commentId) {\n' +
347+
' // You can await here\n' +
348+
'}\n\n' +
349+
'useEffect(() => {\n' +
350+
' fetchComment(commentId);\n' +
351+
'}, [commentId]);\n\n' +
352+
'In the future, React will provide a more idiomatic solution for data fetching ' +
353+
"that doesn't involve writing effects manually.";
354+
} else {
355+
addendum = ' You returned: ' + destroy;
354356
}
357+
warningWithoutStack(
358+
false,
359+
'An Effect function must not return anything besides a function, ' +
360+
'which is used for clean-up.%s%s',
361+
addendum,
362+
getStackByFiberInDevAndProd(finishedWork),
363+
);
355364
}
356-
destroy = null;
357365
}
358-
effect.destroy = destroy;
359366
}
360367
effect = effect.next;
361368
} while (effect !== firstEffect);
@@ -696,7 +703,7 @@ function commitUnmount(current: Fiber): void {
696703
let effect = firstEffect;
697704
do {
698705
const destroy = effect.destroy;
699-
if (destroy !== null) {
706+
if (destroy !== undefined) {
700707
safelyCallDestroy(current, destroy);
701708
}
702709
effect = effect.next;

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,14 @@ export type Dispatcher = {
5959
observedBits: void | number | boolean,
6060
): T,
6161
useRef<T>(initialValue: T): {current: T},
62-
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void,
63-
useLayoutEffect(create: () => mixed, deps: Array<mixed> | void | null): void,
62+
useEffect(
63+
create: () => (() => void) | void,
64+
deps: Array<mixed> | void | null,
65+
): void,
66+
useLayoutEffect(
67+
create: () => (() => void) | void,
68+
deps: Array<mixed> | void | null,
69+
): void,
6470
useCallback<T>(callback: T, deps: Array<mixed> | void | null): T,
6571
useMemo<T>(nextCreate: () => T, deps: Array<mixed> | void | null): T,
6672
useImperativeHandle<T>(
@@ -119,8 +125,8 @@ type HookDev = Hook & {
119125

120126
type Effect = {
121127
tag: HookEffectTag,
122-
create: () => mixed,
123-
destroy: (() => mixed) | null,
128+
create: () => (() => void) | void,
129+
destroy: (() => void) | void,
124130
deps: Array<mixed> | null,
125131
next: Effect,
126132
};
@@ -780,13 +786,13 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
780786
const hook = mountWorkInProgressHook();
781787
const nextDeps = deps === undefined ? null : deps;
782788
sideEffectTag |= fiberEffectTag;
783-
hook.memoizedState = pushEffect(hookEffectTag, create, null, nextDeps);
789+
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
784790
}
785791

786792
function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
787793
const hook = updateWorkInProgressHook();
788794
const nextDeps = deps === undefined ? null : deps;
789-
let destroy = null;
795+
let destroy = undefined;
790796

791797
if (currentHook !== null) {
792798
const prevEffect = currentHook.memoizedState;
@@ -805,7 +811,7 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
805811
}
806812

807813
function mountEffect(
808-
create: () => mixed,
814+
create: () => (() => void) | void,
809815
deps: Array<mixed> | void | null,
810816
): void {
811817
return mountEffectImpl(
@@ -817,7 +823,7 @@ function mountEffect(
817823
}
818824

819825
function updateEffect(
820-
create: () => mixed,
826+
create: () => (() => void) | void,
821827
deps: Array<mixed> | void | null,
822828
): void {
823829
return updateEffectImpl(
@@ -829,7 +835,7 @@ function updateEffect(
829835
}
830836

831837
function mountLayoutEffect(
832-
create: () => mixed,
838+
create: () => (() => void) | void,
833839
deps: Array<mixed> | void | null,
834840
): void {
835841
return mountEffectImpl(
@@ -841,7 +847,7 @@ function mountLayoutEffect(
841847
}
842848

843849
function updateLayoutEffect(
844-
create: () => mixed,
850+
create: () => (() => void) | void,
845851
deps: Array<mixed> | void | null,
846852
): void {
847853
return updateEffectImpl(
@@ -860,7 +866,9 @@ function imperativeHandleEffect<T>(
860866
const refCallback = ref;
861867
const inst = create();
862868
refCallback(inst);
863-
return () => refCallback(null);
869+
return () => {
870+
refCallback(null);
871+
};
864872
} else if (ref !== null && ref !== undefined) {
865873
const refObject = ref;
866874
if (__DEV__) {
@@ -1205,7 +1213,10 @@ if (__DEV__) {
12051213
currentHookNameInDev = 'useContext';
12061214
return mountContext(context, observedBits);
12071215
},
1208-
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
1216+
useEffect(
1217+
create: () => (() => void) | void,
1218+
deps: Array<mixed> | void | null,
1219+
): void {
12091220
currentHookNameInDev = 'useEffect';
12101221
return mountEffect(create, deps);
12111222
},
@@ -1218,7 +1229,7 @@ if (__DEV__) {
12181229
return mountImperativeHandle(ref, create, deps);
12191230
},
12201231
useLayoutEffect(
1221-
create: () => mixed,
1232+
create: () => (() => void) | void,
12221233
deps: Array<mixed> | void | null,
12231234
): void {
12241235
currentHookNameInDev = 'useLayoutEffect';
@@ -1289,7 +1300,10 @@ if (__DEV__) {
12891300
currentHookNameInDev = 'useContext';
12901301
return updateContext(context, observedBits);
12911302
},
1292-
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
1303+
useEffect(
1304+
create: () => (() => void) | void,
1305+
deps: Array<mixed> | void | null,
1306+
): void {
12931307
currentHookNameInDev = 'useEffect';
12941308
return updateEffect(create, deps);
12951309
},
@@ -1302,7 +1316,7 @@ if (__DEV__) {
13021316
return updateImperativeHandle(ref, create, deps);
13031317
},
13041318
useLayoutEffect(
1305-
create: () => mixed,
1319+
create: () => (() => void) | void,
13061320
deps: Array<mixed> | void | null,
13071321
): void {
13081322
currentHookNameInDev = 'useLayoutEffect';
@@ -1376,7 +1390,10 @@ if (__DEV__) {
13761390
warnInvalidHookAccess();
13771391
return mountContext(context, observedBits);
13781392
},
1379-
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
1393+
useEffect(
1394+
create: () => (() => void) | void,
1395+
deps: Array<mixed> | void | null,
1396+
): void {
13801397
currentHookNameInDev = 'useEffect';
13811398
warnInvalidHookAccess();
13821399
return mountEffect(create, deps);
@@ -1391,7 +1408,7 @@ if (__DEV__) {
13911408
return mountImperativeHandle(ref, create, deps);
13921409
},
13931410
useLayoutEffect(
1394-
create: () => mixed,
1411+
create: () => (() => void) | void,
13951412
deps: Array<mixed> | void | null,
13961413
): void {
13971414
currentHookNameInDev = 'useLayoutEffect';
@@ -1471,7 +1488,10 @@ if (__DEV__) {
14711488
warnInvalidHookAccess();
14721489
return updateContext(context, observedBits);
14731490
},
1474-
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
1491+
useEffect(
1492+
create: () => (() => void) | void,
1493+
deps: Array<mixed> | void | null,
1494+
): void {
14751495
currentHookNameInDev = 'useEffect';
14761496
warnInvalidHookAccess();
14771497
return updateEffect(create, deps);
@@ -1486,7 +1506,7 @@ if (__DEV__) {
14861506
return updateImperativeHandle(ref, create, deps);
14871507
},
14881508
useLayoutEffect(
1489-
create: () => mixed,
1509+
create: () => (() => void) | void,
14901510
deps: Array<mixed> | void | null,
14911511
): void {
14921512
currentHookNameInDev = 'useLayoutEffect';

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

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -542,30 +542,40 @@ describe('ReactHooks', () => {
542542
]);
543543
});
544544

545-
it('warns for bad useEffect return values', () => {
545+
it('assumes useEffect clean-up function is either a function or undefined', () => {
546546
const {useLayoutEffect} = React;
547+
547548
function App(props) {
548549
useLayoutEffect(() => {
549550
return props.return;
550551
});
551552
return null;
552553
}
553-
let root;
554554

555-
expect(() => {
556-
root = ReactTestRenderer.create(<App return={17} />);
557-
}).toWarnDev([
558-
'Warning: useEffect function must return a cleanup function or ' +
559-
'nothing.\n' +
560-
' in App (at **)',
555+
const root1 = ReactTestRenderer.create(null);
556+
expect(() => root1.update(<App return={17} />)).toWarnDev([
557+
'Warning: An Effect function must not return anything besides a ' +
558+
'function, which is used for clean-up. You returned: 17',
561559
]);
562560

563-
expect(() => {
564-
root.update(<App return={Promise.resolve()} />);
565-
}).toWarnDev([
566-
'Warning: useEffect function must return a cleanup function or nothing.\n\n' +
561+
const root2 = ReactTestRenderer.create(null);
562+
expect(() => root2.update(<App return={null} />)).toWarnDev([
563+
'Warning: An Effect function must not return anything besides a ' +
564+
'function, which is used for clean-up. You returned null. If your ' +
565+
'effect does not require clean up, return undefined (or nothing).',
566+
]);
567+
568+
const root3 = ReactTestRenderer.create(null);
569+
expect(() => root3.update(<App return={Promise.resolve()} />)).toWarnDev([
570+
'Warning: An Effect function must not return anything besides a ' +
571+
'function, which is used for clean-up.\n\n' +
567572
'It looks like you wrote useEffect(async () => ...) or returned a Promise.',
568573
]);
574+
575+
// Error on unmount because React assumes the value is a function
576+
expect(() => {
577+
root3.update(null);
578+
}).toThrow('is not a function');
569579
});
570580

571581
it('warns for bad useImperativeHandle first arg', () => {

packages/react/src/ReactHooks.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,15 +84,15 @@ export function useRef<T>(initialValue: T): {current: T} {
8484
}
8585

8686
export function useEffect(
87-
create: () => mixed,
87+
create: () => (() => void) | void,
8888
inputs: Array<mixed> | void | null,
8989
) {
9090
const dispatcher = resolveDispatcher();
9191
return dispatcher.useEffect(create, inputs);
9292
}
9393

9494
export function useLayoutEffect(
95-
create: () => mixed,
95+
create: () => (() => void) | void,
9696
inputs: Array<mixed> | void | null,
9797
) {
9898
const dispatcher = resolveDispatcher();

0 commit comments

Comments
 (0)