Remove state from dep array of useStore's useEffect

[antonisprovidakis]

I couldn't figure out any obvious reason for why state was added in the dep array of the useEffect used in useStore, since it's not used inside it.

I removed it and everything keeps working as before (i.e. unit tests pass, manual testing seems just fine).

Actually, things got a bit better since the useEffect does not run for every state update.
It only runs once: on mount.

Before

The useEffect runs 7 times (for each subscriber) for the initial mount and all state transitions of a single toast.

before.mp4

After

The useEffect runs 1 time (for each subscriber) only for the initial mount.

after.mp4

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-hot-toast	✅ Ready (Inspect)	Visit Preview	Oct 1, 2022 at 7:23PM (UTC)

[jluxenberg]

👍 this looks good to me.

Question: should setState be in the deps list instead? I don't think useState ever returns a different "setter" function after the first invocation but it could.

e.g.

  useEffect(() => {
    listeners.push(setState);
    return () => {
      const index = listeners.indexOf(setState);
      if (index > -1) {
         listeners.splice(index, 1);
       }
     };
   }, [setState]);

If we were using eslint we could add the react-hooks/exhaustive-deps rule to check for these issues.

[antonisprovidakis]

I'm pretty sure, react-hooks/exhaustive-deps won't "yell" at us for omitting setState from the dep array.
It's guaranteed (by React) that setState will be stable for the lifetime of the component.

[jluxenberg]

Ah yes you are correct:

React guarantees that setState function identity is stable and won’t change on re-renders. This is why it’s safe to omit from the useEffect or useCallback dependency list.

https://reactjs.org/docs/hooks-reference.html#usestate

[antonisprovidakis]

@timolins If this is not going to be merged for some reason, should I close it then?