Skip to content

Commit

Permalink
Warn for bad useEffect return value (#14069)
Browse files Browse the repository at this point in the history
Mostly to catch this:

```js
useEffect(async () => {
  // ...
  return cleanup;
});
```

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 authored Nov 2, 2018
1 parent ae196e8 commit 293fed8
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 2 deletions.
22 changes: 20 additions & 2 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,26 @@ 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__) {
if (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 293fed8

Please sign in to comment.