Skip to content

Commit 2cb5b8a

Browse files
committed
Assume Effect hook returns either a function or undefined
1 parent 830b3ed commit 2cb5b8a

File tree

3 files changed

+48
-44
lines changed

3 files changed

+48
-44
lines changed

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -320,42 +320,40 @@ 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-
);
354-
}
331+
effect.destroy = create();
332+
333+
if (__DEV__) {
334+
const destroy = effect.destroy;
335+
if (destroy !== undefined && typeof destroy !== 'function') {
336+
warningWithoutStack(
337+
false,
338+
'useEffect function must return a cleanup function or ' +
339+
'nothing (undefined).%s%s',
340+
destroy !== null && typeof destroy.then === 'function'
341+
? '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' +
342+
'Instead, you may write an async function separately ' +
343+
'and then call it from inside the effect:\n\n' +
344+
'async function fetchComment(commentId) {\n' +
345+
' // You can await here\n' +
346+
'}\n\n' +
347+
'useEffect(() => {\n' +
348+
' fetchComment(commentId);\n' +
349+
'}, [commentId]);\n\n' +
350+
'In the future, React will provide a more idiomatic solution for data fetching ' +
351+
"that doesn't involve writing effects manually."
352+
: '',
353+
getStackByFiberInDevAndProd(finishedWork),
354+
);
355355
}
356-
destroy = null;
357356
}
358-
effect.destroy = destroy;
359357
}
360358
effect = effect.next;
361359
} while (effect !== firstEffect);
@@ -696,7 +694,7 @@ function commitUnmount(current: Fiber): void {
696694
let effect = firstEffect;
697695
do {
698696
const destroy = effect.destroy;
699-
if (destroy !== null) {
697+
if (destroy !== undefined) {
700698
safelyCallDestroy(current, destroy);
701699
}
702700
effect = effect.next;

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ type HookDev = Hook & {
126126
type Effect = {
127127
tag: HookEffectTag,
128128
create: () => (() => void) | void,
129-
destroy: (() => void) | null,
129+
destroy: (() => void) | void,
130130
deps: Array<mixed> | null,
131131
next: Effect,
132132
};
@@ -786,13 +786,13 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
786786
const hook = mountWorkInProgressHook();
787787
const nextDeps = deps === undefined ? null : deps;
788788
sideEffectTag |= fiberEffectTag;
789-
hook.memoizedState = pushEffect(hookEffectTag, create, null, nextDeps);
789+
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
790790
}
791791

792792
function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
793793
const hook = updateWorkInProgressHook();
794794
const nextDeps = deps === undefined ? null : deps;
795-
let destroy = null;
795+
let destroy = undefined;
796796

797797
if (currentHook !== null) {
798798
const prevEffect = currentHook.memoizedState;

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

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -542,30 +542,36 @@ 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: useEffect function must return a cleanup function or nothing (undefined).',
561558
]);
562559

563-
expect(() => {
564-
root.update(<App return={Promise.resolve()} />);
565-
}).toWarnDev([
566-
'Warning: useEffect function must return a cleanup function or nothing.\n\n' +
560+
const root2 = ReactTestRenderer.create(null);
561+
expect(() => root2.update(<App return={null} />)).toWarnDev([
562+
'Warning: useEffect function must return a cleanup function or nothing (undefined).',
563+
]);
564+
565+
const root3 = ReactTestRenderer.create(null);
566+
expect(() => root3.update(<App return={Promise.resolve()} />)).toWarnDev([
567+
'Warning: useEffect function must return a cleanup function or nothing (undefined).\n\n' +
567568
'It looks like you wrote useEffect(async () => ...) or returned a Promise.',
568569
]);
570+
571+
// Error on unmount because React assumes the value is a function
572+
expect(() => {
573+
root3.update(null);
574+
}).toThrow('is not a function');
569575
});
570576

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

0 commit comments

Comments
 (0)