[eslint-plugin-react-hooks] async functions should be allowed for custom effects

[vkurchatkin]

Consider the following code:

useAsyncEffect(async () => {
  await foo()
}, []);

Eslint give the following error:

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

useEffect(() => {
  async function fetchData() {
    // You can await here
    const response = await MyAPI.getData(someId);
    // ...
  }
  fetchData();
}, [someId]); // Or [] if effect doesn't need props or state

There are 2 problems with that:

1. The error is provided by react-hooks/exhaustive-deps, but this has nothing to do with deps or exhaustiveness. I can't disable this warning without disable actually checking for exhaustive deps.
2. In general the error is bogus. Statement "Effect callbacks are synchronous to prevent race conditions" is only true for built-in effects, and is definitely not true for useAsyncEffect, which built specifically to support asynchronous callbacks.

eslint-plugin-react-hooks version: 4.0.0

The current behavior

react-hooks/exhaustive-deps gives an error for this code

The expected behavior

react-hooks/exhaustive-deps should not give an error for this code

[threepointone]

https://github.com/facebook/react/blob/master/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L104 is where the check happens.

iiuc, this is to allow passing back a cleanup/cancel function, which is much harder with your abstraction. I hope you've considered those ramifications and have your own workarounds.

I dunno whether the team will change the regex, would be interested to hear from them.

[vkurchatkin]

Just wanted to offer my perspective. I consider it plain wrong to place restrictions like this on my hooks and if this is intended behaviour, my only option is to drop exhaustive-deps completely

[threepointone]

You could also rename your hook. An Effect has a specific shape and meaning in React, so maybe co-opting it might not be the right move.

[vkurchatkin]

An Effect has a specific shape and meaning in React, so maybe co-opting it might not be the right move.

Technically, I could, I've thought of that. But useAsyncEffect is the obvious name for what it does. I don't want to choose objectively worse name because of a linter rule.

[gaearon]

Fixed in eslint-plugin-react-hooks@4.0.3 by removing the flawed heuristic.

[whatisaphone]

@gaearon Unfortunately it appears this isn't quite fixed. It works without additionalHooks, but if you set additionalHooks it still raises the error.

$ yarn list | grep eslint-plugin-react-hooks
├─ eslint-plugin-react-hooks@4.0.3

.eslintrc.json:

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

source:

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

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

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

<snip>/src/file.jsx
  5:18  error  Effect callbacks are synchronous to prevent race conditions. Put the async function inside:

useEffect(() => {
  async function fetchData() {
    // You can await here
    const response = await MyAPI.getData(someId);
    // ...
  }
  fetchData();
}, [someId]); // Or [] if effect doesn't need props or state

Learn more about data fetching with Hooks: https://fb.me/react-hooks-data-fetching  react-hooks/exhaustive-deps

✖ 1 problem (1 error, 0 warnings)

[gaearon]

It works without additionalHooks, but if you set additionalHooks it still raises the error.

That seems like a separate issue. Can you please file a new issue with your suggested proposal?

[gaearon]

I'm beginning to worry that additionalHooks is a problem. It's not scalable that every third party Hook wants its own behavior to be added to configuration. Generally saying, third-party Hooks should avoid dependencies as their primary API.

[whatisaphone]

Can you please file a new issue?

Sure: #19034

---

It's not scalable that every third party Hook wants its own behavior to be added to configuration.

I agree. In my ideal world, instead of making you list the hooks out by hand, the rule would use type info from typescript/flow and check deps for any function with a parameter of type React.DependencyList. Here's useEffect for reference:

function useEffect(effect: EffectCallback, deps?: DependencyList): void;

And looking at that signature, perhaps the lint against async functions could similarly be tied to EffectCallback.

---

Generally saying, third-party Hooks should avoid dependencies as their primary API.

Do you have any thoughts on how you might build something like useAsyncEffect while avoiding a dependency list?