reactjs / react.dev

The React documentation website
https://react.dev/
Creative Commons Attribution 4.0 International
10.87k stars 7.45k forks source link

Third Rule of Hooks: "Side effects may only be caused in useEffect and event handlers" #3560

Open Jonasdoubleyou opened 3 years ago

Jonasdoubleyou commented 3 years ago

I'd like to propose a third rule of hooks.

In the section "Advanced Guides > Strict Mode" it states that

Because the above methods might be called more than once, it’s important that they do not contain side-effects. Ignoring this rule can lead to a variety of problems, including memory leaks and invalid application state.

If I'm not mistaken, the inverse of this statement is that side effects based on a certain state of states shall be triggered in a useEffect, and that side effects from user interaction shall be triggered directly in the event handler. Everything else shall be side effect free. I think that adding this to the "rules of hooks" section makes it easier to remember and gives it much more visibility. I think the "variety of problems" will especially grow when concurrent mode is introduced. By having a "checklist" that can be used during code reviews etc. makes it easier to avoid this kind of problems.

Also one could maybe repeat the "Do not mutate state" rule there, cause this is still a very common problem and having it multiple times in the docs does no harm in my eyes.

What do you think?

konghu commented 3 years ago

Could you help me understand why useEffect is better off in the following case where side effect does not happen within useEffect or event handlers?

With useEffect I can have

function Example() {
  const [count, setCount] = useState(0);

  useEffect(() => {
    document.title = `You clicked ${count} times`;
  });

  return (
    <div>
      <p>You clicked {count} times</p>
      <button onClick={() => setCount(count + 1)}>
        Click me
      </button>
    </div>
  );
}

But I can also simply write

function Example() {
  const [count, setCount] = useState(0);
  document.title = `You clicked ${count} times`;

  return (...);
}

What's the difference?

eps1lon commented 3 years ago

What's the difference?

In concurrent mode React might start rendering a component but not tell the browser to paint it. One example for this would be if Example is rendered inside a Suspense boundary and another component suspends (e.g. because a React.lazy component is being rendered). In that case Example wouldn't be visible to the User but it's side-effect (document.title) would.

I'd argue that it's better to avoid technical debt i.e. stick to the React documentation and use useEffect.

eps1lon commented 3 years ago

Third Rule of Hooks: "Side effects may only be caused in useEffect and event handlers"

This is not exclusive to Hook. This applies to all things you can do in React. An overview about side-effects in React can be found in https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects.

@Jonasdoubleyou Does this clear things up?

Jonasdoubleyou commented 3 years ago

@konghu Well, if you replace document.title = with bookFlight() then this snippet gets way more problematic. I'd say that only a small number of side effects are "harmless" in the way that they can be triggered repeatedly without any problems. And for those few cases, moving them into an useEffect hook is not much overhead.

Jonasdoubleyou commented 3 years ago

@eps1lon I'm sorry, but this is a proposal, not a question. Also I already referenced the exact same section of the docs and explained why I think that the current way of documenting this is not quite optimal (less visible, naming ways to not do it instead of ways how to). I'm also aware that this also applies to the "old class based components", but as far as I know a project is ongoing to rewrite the documentation to be "hook centric". So especially new users will see the "rules of hooks" in the future, and for them I think this information is much more relevant.