facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.59k stars 46.79k forks source link

Issue regarding ESLint react-hooks - 'exhaustive-deps' rule #20258

Closed a-tonchev closed 6 months ago

a-tonchev commented 3 years ago

I read in detail the following issue: https://github.com/facebook/react/issues/14920#issuecomment-467212561

As suggested in the last comment of gaearon I file a new issue because I could not find solution of my cases.

I read really a lot online and in detail about useEffect in general and the exhaustive-deps rule, but somehow there is still a lot of walking in a circle. So I would be very thankful for better suggestion or clarifying how to handle this issues:

It looks that it confuses also a lot of developers and it is quite difficult to write work-arounds for the most common use-cases:

https://github.com/facebook/react/issues/14920

Here I provide few examples that really are not easy to be set-up with this rule:

Example 1: Initialization of app - or check cookies/localstorage once the application loads, and set them in the context:

import React, { useState, createContext, useEffect } from 'react';
import { useTranslation } from 'react-i18next';
import CookieHandler from '../helpers/CookieHandler';
import Loading from '../components/common/Loading';

const defaultValues = {
  authorization: {
    loggedIn: false,
    isAdmin: false,
  },
  dispatchAuthorization: () => {},
};

const LoginContext = createContext(defaultValues);

const reducer = (state, authorization = { loggedIn: false, isAdmin: false }) => {
  const { loggedIn, isAdmin } = authorization;
  return { loggedIn: !!loggedIn, isAdmin: !!isAdmin };
};

const LoginContextProvider = ({ children }) => {
  const [authorization, dispatchAuthorization] = React.useReducer(reducer, defaultValues.authorization);
  const [mounted, setMounted] = useState(false);
  const { i18n } = useTranslation();

  const setLoginData = (token, isAdmin) => {
    if (token) {
      dispatchAuthorization({
        loggedIn: true,
        isAdmin: isAdmin,
      });
    }
    setMounted(true);
  };

  const setLanguage = (language) => {
    i18n.changeLanguage(language);
    CookieHandler.setLanguage(language);
  };

  const loginUser = data => {
    CookieHandler.setToken(data.token);
    CookieHandler.setAdmin(data.isAdmin);
    setLoginData(data.token, data.isAdmin);
  };

  const removeLoginData = () => {
    CookieHandler.logout();
    dispatchAuthorization({
      loggedIn: false,
      isAdmin: false,
    });
  };

  useEffect(() => {
    const token = CookieHandler.getToken();
    const isAdmin = CookieHandler.getAdmin();
    const language = CookieHandler.getLanguage();

    if (token) {
      setLoginData(token, isAdmin);
    } else if (authorization.loggedIn || authorization.isAdmin) removeLoginData();

    if (language) setLanguage(language);
    setMounted(true);
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, []);

  if (!mounted) return <Loading />;

  return (
    <LoginContext.Provider
      value={{
        authorization,
        dispatchAuthorization,
        setLoginData,
        loginUser,
        setLanguage,
        logoutUser: removeLoginData,
      }}
    >
      {children}
    </LoginContext.Provider>
  );
};

const LoginContextConsumer = LoginContext.Consumer;

export { LoginContext, LoginContextProvider, LoginContextConsumer };

Here I want that the useEffect is called really once the application is loaded, and never again. But according to eslint I should add the authorization variable in the dept array, but that is something I really don't want to do. Because each time there is a change in the authorization, the whole INIT-Prozess will start over and over again. I think an init-functionality that fires once the component/or application starts is quite needed, without thinking of changing in variables later?

Example 2: Use of functions that are comming from hooks. An example with react-i18next:

...
import { useTranslation } from 'react-i18next';
...

const MyComponent = ({ someId }) => {
 const { t } = useTranslation();
 const [data, setData] = useState(null);
  useEffect(() => {
    const someAsyncFetch = async () => {
  try {
    const data = await asyncFetchData({
      someId,
    });
    setData(data);
    toastr.success(t('your.data.was.fetched.successfully'));
  } catch (e) {
    console.log(e);
    toastr.error(t('your.data.can.not.be.fetched'));
  }
 };
    someAsyncFetch();
  }, [someId]);
}

In this example, I want to get all the data when my component gets rendered, or when the someId is changed. When data is loaded or error comes - I fire toastr message with the translation function t that I get in a hook. But I don't want to get new data each time the user switch his language. But eslint want me to put t in the deps array, which causes to re-render component and load new data each time the user switch the language. And moreover, we should expect that more and more such hook-functions will come in future, which we eventually may want to use in the useEffects.

Example 3: One variable is depending on other variables

  useEffect(() => {
    if (error === null) {
      const data = doSomethingWithMyValue(value1);
      setMyData(data);
    } else {
      showSomeErrorText(); //or do nothing
    }
  }, [value1]);

If I want that only when value1 changes, the function doSomethingWithMyValue will be fired, but not when error changes. When we put error inside the useEffects deps array then the whole logic is broken, because useEffect will be fired each time error changes. And this is something I don't want.

I tried also to make some kind of workaround with a mounted variable, and useCallback, but it looks for me that useEffect fires unnecessary many times, and somehow the whole thing is slower.

So is there somewhere a logic-catch or we really need to use all this memo and useCallbacks etc. to prevent some serious issue?

Thanks for any feedback!

maxsbelt commented 3 years ago

Hi @a-tonchev. IMO disabling react-hooks/exhaustive-deps rule in some cases is ok (that's why the suggested value for this hook is warn). Another way that you can use to avoid disabling the rule is the usage of useRef hook (can be used in all 3 examples). E.g.:

const errorRef = React.useRef(error);
errorRef.current = error;

useEffect(() => {
  if (errorRef.current === null) {
    const data = doSomethingWithMyValue(value1);
    setMyData(data);
  } else {
    showSomeErrorText(); //or do nothing
  }
}, [value1]);
BGehrels commented 2 years ago

It would be great if one could configure more precisely what this hook does and what not. Example:

A more lightweight solution might be to add a config variable to ignore certain hooks that trigger a lot of false positives:

        'react-hooks/exhaustive-deps': [1, { ignoreHooks: 'useEffect' }],
github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] commented 6 months ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!