Skip to content

Commit

Permalink
Warn for bad useEffect return value
Browse files Browse the repository at this point in the history
Is this too restrictive? Not sure if you would want to do like

```js
useEffect(() => ref.current.style.color = 'red');
```

which would give a false positive here. We can always relax it to only warn on Promises if people complain.
  • Loading branch information
sophiebits committed Nov 2, 2018
1 parent 595b4f9 commit 1fdfae8
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 2 deletions.
19 changes: 17 additions & 2 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,23 @@ function commitHookEffectList(
if ((effect.tag & mountTag) !== NoHookEffect) {
// Mount
const create = effect.create;
const destroy = create();
effect.destroy = typeof destroy === 'function' ? destroy : null;
let destroy = create();
if (typeof destroy !== 'function') {
if (__DEV__ && destroy !== null && destroy !== undefined) {
warningWithoutStack(
false,
'useEffect function must return a cleanup function or nothing.%s%s',
typeof destroy.then === 'function'
? ' Promises and useEffect(async () => ...) are not ' +
'supported, but you can call an async function inside an ' +
'effect.'
: '',
getStackByFiberInDevAndProd(finishedWork),
);
}
destroy = null;
}
effect.destroy = destroy;
}
effect = effect.next;
} while (effect !== firstEffect);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,32 @@ describe('ReactHooks', () => {
]);
expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
});

it('warns for bad useEffect return values', () => {
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 **)',
]);

expect(() => {
root.update(<App return={Promise.resolve()} />);
}).toWarnDev([
'Warning: useEffect function must return a cleanup function or ' +
'nothing. Promises and useEffect(async () => ...) are not supported, ' +
'but you can call an async function inside an effect.\n' +
' in App (at **)',
]);
});
});

0 comments on commit 1fdfae8

Please sign in to comment.