Rooks should use 'useEffect()' as it is intended to be used

[bradharms]

The Problem

I've been researching React's core useEffect() hook and discussing with other developers on my team trying to understand it better. From what I can tell, some of the hooks in Rooks do not use it according to the intentions and paradigms expressed by React in the current version (17) and upcoming version (18). In particular, some of the hooks in Rooks intentionally omit dependencies from useEffect() for various reasons, such as to prevent the callback from running unless specific expressions change or to emulate class component lifecycle methods.

Here is a list of some examples where Rooks is doing this, though this is not exhaustive:

• https://github.com/imbhargav5/rooks/blob/main/src/hooks/useDidMount.ts#L14
• https://github.com/imbhargav5/rooks/blob/main/src/hooks/useDidUpdate.ts#L29
• https://github.com/imbhargav5/rooks/blob/main/src/hooks/useEventListenerRef.ts#L39

Supporting Arguments

With the exception of certain dependencies generated by React core hooks, this pattern is explicitly noted as a pattern to avoid by by numerous sources, most notably the React docs themselves.

React Docs

I acknowledge that the React docs seem to suggest using useEffect() in this way:

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument. This tells React that your effect doesn’t depend on any values from props or state, so it never needs to re-run. This isn’t handled as a special case — it follows directly from how the dependencies array always works.

...BUT. Note what it says in the very next two paragraphs (emphasis added):

If you pass an empty array ([]), the props and state inside the effect will always have their initial values. While passing [] as the second argument is closer to the familiar componentDidMount and componentWillUnmount mental model, there are usually better solutions to avoid re-running effects too often. [...].

We recommend using the exhaustive-deps rule as part of our eslint-plugin-react-hooks package. It warns when dependencies are specified incorrectly and suggests a fix.

Note that use of useEffect() like this intentionally does not make it through the recommended linters, and the solution the React docs recommend is to refactor the code rather than tell the linter to ignore the line.

React 18's "Strict Effects"

Also note that the upcoming React version 18 adds features that try to specifically detect when useEffect() is being used in an unintended way which will trigger unwanted behavior. While I understand that not everybody will be using strict mode, making Rooks require that users not use it is unnecessarily restrictive to them.

Dan Abromov Says Don't

Dan Abromov is the author of Redux and one of the developers of React itself. He posted an article that goes into great depth about why dependencies should always be accurate and how to avoid it.

Recommendation

My recommendation would be to treat useEffect() callbacks like they always runs every render no matter what, and to think of any side effects they perform as the output of a calculation based on current state, which is a calculation that should be safe to perform any number of times.

Closing Comments

I once spent a lot of time and effort arguing against the need for declaring every dependency except those that should trigger re-runs, but once I saw that React 18 planned to reinforce the intended usage with strict effects I realized it was not safe to assume the behavior of implementation because it is implied that the React dev team reserves the right to change that behavior at some point in the future.

[imbhargav5]

This is a great discussion point and thanks for this lovely write up @bradharms. I see the point you are making and I am going to dwell on this a bit more before thinking of next steps!

Of course, if you would like, please feel free to send in a PR. But no stress, if you are not able to, me or @qiweiii will get to the bottom of this.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.