facebook / react

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

Try to mock didMount with useEffect, but keep getting weird warning (20 line codesandbox inside) #17713

Closed FateRiddle closed 4 years ago

FateRiddle commented 4 years ago

Current behavior

We are supposed to do

useEffect(()=> {
   loadData()
}, [])

to mock the behavior of didMount life cycle. But this keeps giving me warning:

React Hook useEffect has a missing dependency: 'loadData'. Either include it or remove the dependency array.

Which doesn't make any sense, and if I follow it, more warnings to come. This is so confusing to say the least.

Here's the codesandbox demo, https://codesandbox.io/s/gracious-feather-neddw and the warning is on line 12.

Edit: I read the whole https://reactjs.org/docs/hooks-faq.html#performance-optimizations part, and understand what causes the warning. But I can't move the function inside because it is used else where, and I absolutely needed loadList to run just once, so adding more dependencies to the [] is not a solution for me. Or any of the solution provided in the doc.

[] basically means it runs only once, like in didMount, why having props & state inside it will be considered unsafe?

function Example({ someProp }) {
  function doSomething() {
    console.log(someProp);
  }

  useEffect(() => {
    doSomething();
  }, []); // 🔴 This is not safe (it calls `doSomething` which uses `someProp`)
}

The bottom line is, if I do this in class component / didMount, it will absolutely work(and safe). But in function component / useEffect, why do we have to try so hard? Is there a design problem?

zxh19890103 commented 4 years ago

If the function loadData varys during the component's lifecycle, you do need the useEffect to see it by passing it in the array. If you want useEffect run only once in the whole lifecycle, you shouldn't use any variables in or you are sure that loadData would never change in lifecycle.

FateRiddle commented 4 years ago

@zxh19890103 say the common table with searchKeys (like name / id / address) and "search" button interface. So loadList will change depend on different searchKeys. You wanna loadList once when the table mounted, which isn't too much to ask, right? Then searchKeys are definitely gonna change depend on user inputs, but you really don't want it to trigger loadList unless you press the "search" button.

This is really a very common scenario.

And honestly I find it is not practical to ask "you shouldn't use any variables in or you are sure that loadData would never change in lifecycle."

bvaughn commented 4 years ago

You may find Dan's [blog post](https://overreacted.io/a-complete-guide-to-useeffect/ about this topic useful. In particular this bit:

Question: How do I correctly fetch data inside useEffect? What is []?

This article is a good primer on data fetching with useEffect. Make sure to read it to the end! It’s not as long as this one. [] means the effect doesn’t use any value that participates in React data flow, and is for that reason safe to apply once. It is also a common source of bugs when the value actually is used. You’ll need to learn a few strategies (primarily useReducer and useCallback) that can remove the need for a dependency instead of incorrectly omitting it.

That being said, the lint rule exists to help you avoid making mistakes, so it warns you that you have a potential stale dependency bug. In most cases, this warning is important and ignoring it can lead to brittle code or bugs down the road. 😄

In your specific case, if you want to ignore the warning, you could add a lint ignore comment to disable the rule:

// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => {
  loadData();
}, []);

If you want to code a bit more defensively (in case this is a shared/reused component) you could do something like this:

function Example({ loadData }) {
  const didMountRef = useRef(false);

  useEffect(() => {
    if (didMountRef.current) {
      // Warn since loadData() prop changed after mount
    } else {
      didMountRef.current = true;
      loadData();
    }
  }, [loadData]);
}