Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Restrict effect return type to a function or nothing #14119

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Restrict effect return type to a function or nothing #14119

Changes from 3 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
Original file line number Diff line number Diff line change
@@ -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();
@@ -159,7 +159,7 @@ function useLayoutEffect(
}

function useEffect(
create: () => mixed,
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
): void {
nextHook();
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
@@ -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';
56 changes: 27 additions & 29 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
@@ -320,42 +320,40 @@ 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') {
warningWithoutStack(
false,
'useEffect function must return a cleanup function or ' +
'nothing (undefined).%s%s',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there other common examples we can include here? So it doesn't sounds like a jargon/technical error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add a branch specifically for null

destroy !== null && 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),
);
}
destroy = null;
}
effect.destroy = destroy;
}
effect = effect.next;
} while (effect !== firstEffect);
@@ -696,7 +694,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;
58 changes: 39 additions & 19 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
@@ -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>(
@@ -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,
};
@@ -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;
@@ -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(
@@ -817,7 +823,7 @@ function mountEffect(
}

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

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

function updateLayoutEffect(
create: () => mixed,
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(
@@ -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__) {
@@ -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);
},
@@ -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';
@@ -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);
},
@@ -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';
@@ -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);
@@ -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';
@@ -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);
@@ -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';
30 changes: 18 additions & 12 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
View file
Original file line number Diff line number Diff line change
@@ -542,30 +542,36 @@ 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: useEffect function must return a cleanup function or nothing (undefined).',
]);

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: useEffect function must return a cleanup function or nothing (undefined).',
]);

const root3 = ReactTestRenderer.create(null);
expect(() => root3.update(<App return={Promise.resolve()} />)).toWarnDev([
'Warning: useEffect function must return a cleanup function or nothing (undefined).\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', () => {
4 changes: 2 additions & 2 deletions packages/react/src/ReactHooks.js
View file
Original file line number Diff line number Diff line change
@@ -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();