Skip to content

Restrict effect return type to a function or nothing #14119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Restrict effect return type to a function or nothing #14119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8716f51
Restrict effect return type to a function or nothing
acdlite Nov 6, 2018
830b3ed
Restrict return type further
acdlite Jan 17, 2019
2cb5b8a
Assume Effect hook returns either a function or undefined
acdlite Jan 30, 2019
bec016d
Tweak warning message
acdlite Jan 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-debug-tools/src/ReactDebugHooks.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebmarkbage Usually we coerce missing values to null before storing them in our internal data structures, but I think that's because we usually accept either, and null is preferred because it's less likely to be unintentional. But in this case, since we don't accept null, I can skip the type check in prod. Let me know if this doesn't make sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there has been times where V8 has treated undefined as effectively a missing property in the hidden class rather than a reified value. So setting to undefined might mess with the hidden class. Not sure though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll leave it like this until we learn more, I suppose


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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,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