Bug: exhaustive-deps false positives?

This issue has been tracked since 2022-09-07.

This is a re-opening of #22581. I had to step away from the project for a while and so I didn't respond before it was closed, my apologies. As I mentioned there I did look through the various suggestions in #14920 and nothing seemed to be relevant to my case.

The reply in my original issue helped me fix a few things, but the linter still suggests things that break the code, and I'm still unclear on a few parts of the reply.

Refresher: I have a Component, StatusBox. All it does is pop up a little overlay to tell you that, for example, something was submitted. If there's a timeout, after a timeout, it fades away, otherwise it stays until someone clicks on it.

As it stands my code now looks like:

let innerTimeout
let outerTimeout

/*
 * A box for appearing status messages
 */
const StatusBox = ({content, setContent, timeout, type}) => {
    const div = React.useRef(null)
    const [opacity, setOpacity] = React.useState(0)
    const [className, setClassName] = React.useState(type)

    const handleClick = e => {
        e.preventDefault()
        clear()
    }

    const clear = React.useCallback((e) => {
        console.debug("StatusBox: Clearing opacity, currently:", opacity)
        setOpacity(0)
        /*
         * If we remove the class now, it'll cut short the fade-out.
         * If we remove the text now, it'll disappear abruptly before the
         * fade-out.
         *
         * HOWEVER, if we DON'T clear the text, then the overlay is still
         * there, blocking the page (in the case of a bigger status), so
         * we need to clear it after the transition. So get the timing of
         * that, and add 50ms just to be safe. And while we're at it,
         * we can clear the classes...
         */
        let t = window.getComputedStyle(div.current).transitionDuration
        console.debug("StatusBox: computed transition", t)
        // that's something like 0.4s, we need to drop the 's', convert
        // to a number, and the turn into milliseconds. Trying to use a
        // string in a math equation causes JS to cast it for you
        t = t.replace('s', '') * 1000
        t += 50
        console.debug("StatusBox: Setting timeout to clear type/content in", t)
        innerTimeout = window.setTimeout(() => {
            setContent(null)
        }, t)
    }, [setContent, opacity])

    React.useEffect(() => {
        console.debug(
            `StatusBox: in useEffect, timeout ${timeout}`
        )
        // If we're setting content to null, we've already set the opacity
        // so we can return null to not re-render
        if (content == null) {
            return null
        }

        // otherwise, we need to appear!
        console.debug("StatusBox: setting opacity to 1")
        setOpacity(1)

        // if we have no timeout, we're done
        if (timeout == null) {
            return
        }

        // otherwise, set the timeout
        console.debug('StatusBox: setting timeout for', timeout)
        // nuke any timeout already there
        if (innerTimeout) {
            console.debug("clearing inner:", innerTimeout)
            window.clearTimeout(innerTimeout)
        }
        if (outerTimeout) {
            console.debug("clearing outer:", innerTimeout)
            window.clearTimeout(outerTimeout)
        }
        outerTimeout = window.setTimeout(clear, timeout)
    }, [content])

    return <div
        className={type}
        ref={div}
        id='status'
        style={{opacity:opacity}}
        onClick={handleClick}
    >{content}</div>
}

This takes into account the suggestion to move clear to be a useCallback. However, now the linter wants both opacity and setContent in the deps list of the useCallback, and then content, clear, and timeout, in the deps list of useEffect. That leads to an infinite loop in the clearing of the box and it never clears.

I think @keanemind was trying to address that with:

To fix the stale clear problem, you can put clear into the dependencies array and then have your effect clean up by cancelling the outerTimeout. Now, when clear changes, the timeout with the stale clear will be cancelled, and then the effect will run again. And by using useCallback on clear, you can have clear change only when setContent changes.

But I didn't follow that. I... do clear outerTimeout in the useEffect already. I tried moving the clear to the very top of the useEffect in case you meant I might not clear it in some cases, but that didn't fix it.

@keanemind also asked:

By the way, is there a reason you don't unmount the overlay after the animation is over, rather than setting the content to null? That part is a little odd to me.

I briefly used class components when I was learning React, but quickly found that the recommendation was to use function components, and in function components I ... don't know how to unmount things.

But also, if I unmounted it, wouldn't that <StatusBox> not be there the next time setContent was called?

Thanks, and apologies in advance if I'm missing obvious stuff

More Details About Repo
Owner Name facebook
Repo Name react
Full Name facebook/react
Language JavaScript
Created Date 2013-05-24
Updated Date 2022-10-05
Star Count 195628
Watcher Count 6648
Fork Count 40518
Issue Count 1111

YOU MAY BE INTERESTED

Issue Title Created Date Updated Date