exhaustive-deps: custom effects should support async functions

[whatisaphone]

We have a custom hook, useAsyncEffect, which works like useEffect except it accepts an async function. We would like to check the deps of this function using exhaustive-deps, but that lint rule is going a bit beyond its name and also checking the type of function we pass.

The exhaustive-deps rule can't know anything about the semantics of arbitrary third-party hooks, so I think it's overstepping its bounds a bit (at least if you take the name "exhaustive deps" literally)

React version: 16.13.1 eslint version: 7.0.0 eslint-plugin-react-hooks version: 4.0.3

Steps To Reproduce

Source:

import React from 'react';
import { useAsyncEffect } from './utilities/react';

function MyComponent() {
  useAsyncEffect(async () => {
    await Promise.resolve()
  }, []);
  return <div />;
}

.eslintrc.json:

{
  "parserOptions": {
    "ecmaVersion": 2020,
    "sourceType": "module",
    "ecmaFeatures": {
      "jsx": true
    }
  },
  "plugins": ["react-hooks"],
  "rules": {
    "react-hooks/exhaustive-deps": [
      "error",
      {
        "additionalHooks": "^useAsyncEffect$"
      }
    ]
  }
}

Then run:

node_modules/.bin/eslint --ext jsx src/file.jsx

The current behavior

  5:18  error  Effect callbacks are synchronous to prevent race conditions. Put the async function inside:

<snip>

✖ 1 problem (1 error, 0 warnings)

The expected behavior

There should be no lint error

[MichaelBelousov]

I'd like to bump this, custom effects using dependencies have their own problems, but if you're choosing to support an additionalHooks option for the linter rule, it shouldn't restrict the argument specification if it can be helped. Perhaps a fork with support for @typescript-eslint (and whatever the flow alternative is) would make the most sense for people that want this, unless this ever gets favored for support.

[mbkv]

:+1:

this rule is really overstepping its boundaries. For reference, I've designed an async effect to be without race conditions

useAsyncEffect(async rerendered => {
  await Promise.resolve(undefined);

  if (rerendered.current) {
    setState(whatever);
  }
}, [deps]);

it's much less verbose than the suggested way, and does exactly the same thing

useEffect(() => {
  let cancel = false;
  async function doStuff() {
    await Promise.resolve(undefined);

    if (!cancel) {
      setState(whatever);
    }
  }

  // while writing this. I had a bug where I forgot to even call it. showing this way is more fragile
  doStuff();

  return () => {
    cancel = true;
  }
}, [deps]);

Edit (August 2022): While I still think the current rule is really overstepping boundaries. The above code is really naive and I leave it for historical reasons. You should instead write an effect using an AbortController or AbortSignal as an argument instead

This lets you cancel fetches instead of forcing the server to fulfill a request and then throw it away. And in personal code you'll just use a try { } finally { } block to clean up whatever state you have

[MichaelBelousov]

I made a minimal PR for this. Ideally imo, additionalHooks would support any arguments for a custom hook, with a convention that the last one is always a dependency list, but I don't think I should make that decision. This PR only stops worrying about whether a function is async for just configured addtionalHooks

[Dawnthorn]

I wonder if it would make more sense to move the effect callbacks async check into a separate rule like react-hooks/async-use-effect? In this case we want to check the dependencies, but we don't want the async check. If they were separate rules we could configure them separately.

[cgriebel]

The suggestion to split them into 2 rules I think is ideal. It also allows suppressing one without the other when needed. I'm forced to decide between suppressing the false positive and risk dependencies being missed or having a bunch of warnings as it currently stands.

[pting-me]

I also agree with the proposal for separate rules. When I turn on exhaustive-deps I don't really expect any messages on async usage. They seem like completely different concerns to me.

[pting-me]

At the moment, the rule runs a check for isEffect which is essentially a regex match for Effect. Rather than assuming that effect hooks will be named use.*Effect, I think the rule should be generalized in case we want to check for async in other effect hooks. Perhaps the rule can be something like react-hooks/no-async-effect, with an additionalHooks parameter like that in exhaustive-deps. Default can be use.*Effect.

[devshorts]

Any traction on this? This is pretty annoying

[freewind]

2 years

[pting-me]

#18861 would likely fix this as well

[ahoisl]

Without knowing this issue, I made a PR fixing the initial problem by adding another configuration parameter, see #24958