Warn for bad useEffect return value #14069
Conversation
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 595b4f9...3f368be react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
This confirms what I suspected all along 😇
I was especially hesitant to allow people to return Promise because I would not be able to detect that they were incorrectly returning a cleanup function from a Promise in TypeScript.
It would be nice to have some guidance as to what's the right approach when you do need to run an async task in an effect, though, other than just invoking an async IIFE inside the callback. Perhaps it's something that can be solved with ConcurrentMode (throw an async IIFE)?
I also think it's bad practice to have an implicit return when none were intended. If you're transpiling to ES5 you're going to have to pay an extra return keyword too.
| getStackByFiberInDevAndProd(finishedWork), | ||
| ); | ||
| } | ||
| destroy = null; |
There was a problem hiding this comment.
In production the branch if (typeof destroy !== 'function') { will still be present with destroy = null. Is it worth keeping it since the next line checks the type again?
There was a problem hiding this comment.
Thanks for the catch, I intended to remove the conditional from the next line.
sophiebits
force-pushed
the
effect-brv
branch
from
fc2a11b to
8b34843
Compare
|
I would like to also know what's the recommended approach for running async code in the effect. I am so used to have What about adding Also would be so bad to actually support Promise? I mean with all that concurrent stuff happening I would be a really surprised if React would be unable to handle asynchronous effects. In case it's planned then this warning should mention it. |
|
The intended solution for most async stuff is Suspense. It's unfortunate we don't have both yet so at first stages people are going to be doing |
| effect.destroy = typeof destroy === 'function' ? destroy : null; | ||
| let destroy = create(); | ||
| if (typeof destroy !== 'function') { | ||
| if (__DEV__ && destroy !== null && destroy !== undefined) { |
There was a problem hiding this comment.
Nit: I kinda perfer if (__DEV__) to be a separate line for easier visual scannning
| false, | ||
| 'useEffect function must return a cleanup function or nothing.%s%s', | ||
| typeof destroy.then === 'function' | ||
| ? ' Promises and async functions are not supported.' |
There was a problem hiding this comment.
Maybe add "but you can call an async function without waiting for it" or something. Or I expect we'll keep getting issues asking for this.
I am still not sure what should we do at this moment? Running async function without |
|
I should probably point out that my intention is not to have function Component() {
const [data, setData] = useState(null)
useEffect(async () => {
const data = await fetchData()
setData(data)
// in this contrieved example I could probably do it like this
fetchData().then(setData)
// but it's usually more complicated than that and async/await is superb sugar
}, [])
}I can imagine this will be much prettier with Suspense, but since we can wrap this into a custom hook then changing to suspensified solution should be fairly easy, right? |
|
@FredyC Maybe you could try this. function Component() {
const [data, setData] = useState(null)
useEffect(() => {
async function fetchAndDoStuffWithData() {
const data = await fetchData()
// do stuff here...
setData(data);
}
fetchAndDoStuffWithData();
})
} |
sophiebits
force-pushed
the
effect-brv
branch
from
8b34843 to
1fdfae8
Compare
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.
sophiebits
force-pushed
the
effect-brv
branch
from
1fdfae8 to
3f368be
Compare
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.
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.
|
I've pretty much settled with a custom hook. It has an obvious downside that I cannot use cleanup function there, but so far I haven't seen a case for an async effect that would need a cleanup. It just reads much better like this imo. const Component = () => {
const [data, setData] = useState(null)
useAsyncEffect(async () => {
const data = await fetchData();
// do stuff here...
setData(data);
}, [])
} |
|
I created a small reusable hook, ready to copy: import { useEffect } from "react";
/**
* Like useEffect but works with async functions and makes sure that errors will be reported
*/
export function useAsyncEffect(effect: () => Promise<any>) {
useEffect(() => {
effect().catch(e => console.warn("useAsyncEffect error", e));
});
} |
|
Something like this would be nicer: But it throws warnings: |
|
@FredyC What does your custom hook useAsyncEffect look like? |
|
@bmmpt Do you realize those warnings are just from friendly neighbor ESLint and you can disable those per line? There is nothing else to be done here. |
|
It would be nice if react hooks ESLint plugin was smart enough to suppress them if they're false positives. I'm not a big fan of disabling ESLint per line, unless there really is no other option. |
|
@bmmpt I've never written an eslint plugin, but I can imagine it's not so easy to write heuristics that are so smart to avoid any false positives. Most importantly, you are a developer, you need to be thinking when writing code. Do not rely on a tool to do a job for you, otherwise, it might bite you. Every ESLint autofix should be validated if it makes sense. The |
Mostly to catch this:
Is this too restrictive? Not sure if you would want to do like
which would give a false positive here. We can always relax it to only warn on Promises if people complain.