Skip to content

Commit

Permalink
Restrict effect return type to a function or nothing (#14119)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@acdlite
acdlite authored Jan 31, 2019
1 parent 51c0791 commit 66eb293
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 65 deletions.
4 changes: 2 additions & 2 deletions packages/react-debug-tools/src/ReactDebugHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ function useRef<T>(initialValue: T): {current: T} {
}

function useLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
): void {
nextHook();
Expand All @@ -159,7 +159,7 @@ function useLayoutEffect(
}

function useEffect(
create: () => mixed,
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
): void {
nextHook();
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/server/ReactPartialRendererHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,8 @@ function useRef<T>(initialValue: T): {current: T} {
}

export function useLayoutEffect(
create: () => mixed,
deps: Array<mixed> | void | null,
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
) {
if (__DEV__) {
currentHookNameInDev = 'useLayoutEffect';
Expand Down
63 changes: 35 additions & 28 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,42 +320,49 @@ function commitHookEffectList(
if ((effect.tag & unmountTag) !== NoHookEffect) {
// Unmount
const destroy = effect.destroy;
effect.destroy = null;
if (destroy !== null) {
effect.destroy = undefined;
if (destroy !== undefined) {
destroy();
}
}
if ((effect.tag & mountTag) !== NoHookEffect) {
// Mount
const create = effect.create;
let destroy = create();
if (typeof destroy !== 'function') {
if (__DEV__) {
if (destroy !== null && destroy !== undefined) {
warningWithoutStack(
false,
'useEffect function must return a cleanup function or ' +
'nothing.%s%s',
typeof destroy.then === 'function'
? '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' +
'Instead, you may write an async function separately ' +
'and then call it from inside the effect:\n\n' +
'async function fetchComment(commentId) {\n' +
' // You can await here\n' +
'}\n\n' +
'useEffect(() => {\n' +
' fetchComment(commentId);\n' +
'}, [commentId]);\n\n' +
'In the future, React will provide a more idiomatic solution for data fetching ' +
"that doesn't involve writing effects manually."
: '',
getStackByFiberInDevAndProd(finishedWork),
);
effect.destroy = create();

if (__DEV__) {
const destroy = effect.destroy;
if (destroy !== undefined && typeof destroy !== 'function') {
let addendum;
if (destroy === null) {
addendum =
' You returned null. If your effect does not require clean ' +
'up, return undefined (or nothing).';
} else if (typeof destroy.then === 'function') {
addendum =
'\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' +
'Instead, you may write an async function separately ' +
'and then call it from inside the effect:\n\n' +
'async function fetchComment(commentId) {\n' +
' // You can await here\n' +
'}\n\n' +
'useEffect(() => {\n' +
' fetchComment(commentId);\n' +
'}, [commentId]);\n\n' +
'In the future, React will provide a more idiomatic solution for data fetching ' +
"that doesn't involve writing effects manually.";
} else {
addendum = ' You returned: ' + destroy;
}
warningWithoutStack(
false,
'An Effect function must not return anything besides a function, ' +
'which is used for clean-up.%s%s',
addendum,
getStackByFiberInDevAndProd(finishedWork),
);
}
destroy = null;
}
effect.destroy = destroy;
}
effect = effect.next;
} while (effect !== firstEffect);
Expand Down Expand Up @@ -696,7 +703,7 @@ function commitUnmount(current: Fiber): void {
let effect = firstEffect;
do {
const destroy = effect.destroy;
if (destroy !== null) {
if (destroy !== undefined) {
safelyCallDestroy(current, destroy);
}
effect = effect.next;
Expand Down
58 changes: 39 additions & 19 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,14 @@ export type Dispatcher = {
observedBits: void | number | boolean,
): T,
useRef<T>(initialValue: T): {current: T},
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void,
useLayoutEffect(create: () => mixed, deps: Array<mixed> | void | null): void,
useEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void,
useLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void,
useCallback<T>(callback: T, deps: Array<mixed> | void | null): T,
useMemo<T>(nextCreate: () => T, deps: Array<mixed> | void | null): T,
useImperativeHandle<T>(
Expand Down Expand Up @@ -119,8 +125,8 @@ type HookDev = Hook & {

type Effect = {
tag: HookEffectTag,
create: () => mixed,
destroy: (() => mixed) | null,
create: () => (() => void) | void,
destroy: (() => void) | void,
deps: Array<mixed> | null,
next: Effect,
};
Expand Down Expand Up @@ -780,13 +786,13 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
const hook = mountWorkInProgressHook();
const nextDeps = deps === undefined ? null : deps;
sideEffectTag |= fiberEffectTag;
hook.memoizedState = pushEffect(hookEffectTag, create, null, nextDeps);
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
}

function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
const hook = updateWorkInProgressHook();
const nextDeps = deps === undefined ? null : deps;
let destroy = null;
let destroy = undefined;

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

function mountEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return mountEffectImpl(
Expand All @@ -817,7 +823,7 @@ function mountEffect(
}

function updateEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(
Expand All @@ -829,7 +835,7 @@ function updateEffect(
}

function mountLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return mountEffectImpl(
Expand All @@ -841,7 +847,7 @@ function mountLayoutEffect(
}

function updateLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(
Expand All @@ -860,7 +866,9 @@ function imperativeHandleEffect<T>(
const refCallback = ref;
const inst = create();
refCallback(inst);
return () => refCallback(null);
return () => {
refCallback(null);
};
} else if (ref !== null && ref !== undefined) {
const refObject = ref;
if (__DEV__) {
Expand Down Expand Up @@ -1205,7 +1213,10 @@ if (__DEV__) {
currentHookNameInDev = 'useContext';
return mountContext(context, observedBits);
},
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
useEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useEffect';
return mountEffect(create, deps);
},
Expand All @@ -1218,7 +1229,7 @@ if (__DEV__) {
return mountImperativeHandle(ref, create, deps);
},
useLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useLayoutEffect';
Expand Down Expand Up @@ -1289,7 +1300,10 @@ if (__DEV__) {
currentHookNameInDev = 'useContext';
return updateContext(context, observedBits);
},
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
useEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useEffect';
return updateEffect(create, deps);
},
Expand All @@ -1302,7 +1316,7 @@ if (__DEV__) {
return updateImperativeHandle(ref, create, deps);
},
useLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useLayoutEffect';
Expand Down Expand Up @@ -1376,7 +1390,10 @@ if (__DEV__) {
warnInvalidHookAccess();
return mountContext(context, observedBits);
},
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
useEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useEffect';
warnInvalidHookAccess();
return mountEffect(create, deps);
Expand All @@ -1391,7 +1408,7 @@ if (__DEV__) {
return mountImperativeHandle(ref, create, deps);
},
useLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useLayoutEffect';
Expand Down Expand Up @@ -1471,7 +1488,10 @@ if (__DEV__) {
warnInvalidHookAccess();
return updateContext(context, observedBits);
},
useEffect(create: () => mixed, deps: Array<mixed> | void | null): void {
useEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useEffect';
warnInvalidHookAccess();
return updateEffect(create, deps);
Expand All @@ -1486,7 +1506,7 @@ if (__DEV__) {
return updateImperativeHandle(ref, create, deps);
},
useLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
currentHookNameInDev = 'useLayoutEffect';
Expand Down
34 changes: 22 additions & 12 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,30 +542,40 @@ describe('ReactHooks', () => {
]);
});

it('warns for bad useEffect return values', () => {
it('assumes useEffect clean-up function is either a function or undefined', () => {
const {useLayoutEffect} = React;

function App(props) {
useLayoutEffect(() => {
return props.return;
});
return null;
}
let root;

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

expect(() => {
root.update(<App return={Promise.resolve()} />);
}).toWarnDev([
'Warning: useEffect function must return a cleanup function or nothing.\n\n' +
const root2 = ReactTestRenderer.create(null);
expect(() => root2.update(<App return={null} />)).toWarnDev([
'Warning: An Effect function must not return anything besides a ' +
'function, which is used for clean-up. You returned null. If your ' +
'effect does not require clean up, return undefined (or nothing).',
]);

const root3 = ReactTestRenderer.create(null);
expect(() => root3.update(<App return={Promise.resolve()} />)).toWarnDev([
'Warning: An Effect function must not return anything besides a ' +
'function, which is used for clean-up.\n\n' +
'It looks like you wrote useEffect(async () => ...) or returned a Promise.',
]);

// Error on unmount because React assumes the value is a function
expect(() => {
root3.update(null);
}).toThrow('is not a function');
});

it('warns for bad useImperativeHandle first arg', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/ReactHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,15 @@ export function useRef<T>(initialValue: T): {current: T} {
}

export function useEffect(
create: () => mixed,
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
) {
const dispatcher = resolveDispatcher();
return dispatcher.useEffect(create, inputs);
}

export function useLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
) {
const dispatcher = resolveDispatcher();
Expand Down

0 comments on commit 66eb293

Please sign in to comment.