Is not including all dependencies in the useEffect dependency array an anti-pattern?

Question

I am trying to wrap my head around the React Hooks API. Specifically, I'm trying to construct the classic use case that once was the following:

componentDidUpdate(prevProps) {
    if (prevProps.foo !== this.props.foo) {
        // Animate DOM elements here...
        this.animateSomething(this.ref, this.props.onAnimationComplete);
    }
}

Now, I tried to build the same with a functional component and useEffect, but can't figure out how to do it. This is what I tried:

useEffect(() => {
    animateSomething(ref, props.onAnimationComplete);
}, [props.foo]);

This way, the effect is only called when props.foo changes. And that does work – BUT! It appears to be an anti-pattern since the eslint-plugin-react-hooks rule warns about this. All dependencies that are used inside the effect should be declared in the dependencies array. So that means I would have to do the following:

useEffect(() => {
    animateSomething(ref, props.onAnimationComplete);
}, [props.foo, ref, props.onAnimationComplete]);

That does not lead to the linting error BUT it totally defeats the purpose of only calling the effect when props.foo changes. I DON'T want it to be called when the other props or the ref change.

Now, I read something about using useCallback to wrap this. I tried it but didn't get any further.

I guess that animation should not be triggered when props.onAnimationComplete changes. Furthermore already running animateSomething still will use an old callback. — UjinT34
– UjinT34, Commented Mar 18, 2019 at 21:38

Ryan Cogswell · Accepted Answer · 2019-03-19 11:24:23Z

38

I would recommend writing this as follows:

const previousFooRef = useRef(props.foo);

useEffect(() => {
    if (previousFooRef.current !== props.foo) {
       animateSomething(ref, props.onAnimationComplete);
       previousFooRef.current = props.foo;
    }
}, [props.foo, props.onAnimationComplete]);

You can't avoid the complexity of having a condition inside the effect, because without it you will run your animation on mount rather than just when props.foo changes. The condition also allows you to avoid animating when things other than props.foo change.

By including props.onAnimationComplete in the dependencies array, you avoid disabling the lint rule which helps ensure that you don’t introduce future bugs related to missing dependencies.

Here's a working example:

edited Mar 19, 2019 at 11:24

answered Mar 19, 2019 at 2:46

Ryan Cogswell

81.8k10 gold badges242 silver badges214 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

UjinT34 Over a year ago

The callback should be recreated every render. And changes to props.onAnimationComplete should trigger a rerender. So there is no real need for this complexity.

Ryan Cogswell Over a year ago

What complexity are you referring to? The condition is necessary to avoid animating on mount. Adding the callback to the dependencies is less complex than disabling the lint rule. It is true that it is not necessary in the dependency array currently, but there would be numerous ways this code could evolve over time where it would matter and disabling the lint rule would make future bugs more likely.

overdub60 Over a year ago

Thanks, that helped me get to the solution!

gmoniava Over a year ago

@RyanCogswell won't linter complain that you omitted ref from dep array?

Ryan Cogswell Over a year ago

@giorgim The linter knows that the value returned from useRef does not change across renders, so refs do not need to be in the dependency array. In the sample sandbox, you can see that there are no eslint warnings, but you do get a warning if you remove foo from the dependency array.

Finesse · Accepted Answer · 2019-12-18 04:57:51Z

4

Suppress the linter because it gives you a bad advice. React requires you to pass to the second argument the values which (and only which) changes must trigger an effect fire.

useEffect(() => {
    animateSomething(ref, props.onAnimationComplete);
}, [props.foo]); // eslint-disable-line react-hooks/exhaustive-deps

It leads to the same result as the Ryan's solution.

I see no problems with violating this linter rule. In contrast to useCallback and useMemo, it won't lead to errors in common case. The content of the second argument is a high level logic.

You may even want to call an effect when an extraneous value changes:

useEffect(() => {
    alert(`Hi ${props.name}, your score is changed`);
}, [props.score]);

edited Dec 18, 2019 at 4:57

answered Dec 18, 2019 at 4:44

Finesse

11k8 gold badges72 silver badges98 bronze badges

2 Comments

Michael Over a year ago

The reason for this linter rule is the following in your examples: 1) if animateSomething ever changed (i.e. you decide to change the animation speed, ...), this effect would still refer to the old function and you will most probably get an unintended result. 2) If the name changes, the user would still see the old name. - Maybe in the case of an alert, you should return a clean-up function that closes this alert, so that you will only show the relevant alert.

Andrew Over a year ago

While it may be not be necessary for this example, I have found cases where, at least to me, it does seem much more desirable to override the linter than work around the behavior, particularly in the case where you wish to limit the effect to the first render by passing an empty array - which still triggers the exhaustive-deps warning.

Finesse · Accepted Answer · 2019-12-18 06:36:13Z

-1

Move the values, that must be fresh (not stale) in the callback but mustn't refire the effect, to refs:

const elementRef = useRef(); // Ex `ref` from the question
const animationCompleteRef = useRef();

animationCompleteRef.current = props.onAnimationComplete;

useEffect(() => {
    animateSomething(elementRef, animationCompleteRef.current);
}, [props.foo, elementRef, animationCompleteRef]);

It works because useRef return value doesn't change on renders.

edited Dec 18, 2019 at 6:36

answered Dec 18, 2019 at 5:14

Finesse

11k8 gold badges72 silver badges98 bronze badges

Collectives™ on Stack Overflow

Is not including all dependencies in the useEffect dependency array an anti-pattern?

3 Answers 3

5 Comments

2 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

5 Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related