Use ref for exiting children (Fixes #1914)

[hluisson]

Fixes #1914

The onExit callback for exiting children could have stale values for the set of exiting keys. This would lead to the last "exiting" element to not realize they were the last exiting element, and therefore not trigger a force render (specific code below)

if (!exiting.size) {
                presentChildren.current = filteredChildren

                if (isMounted.current === false) return

                forceRender()
                onExitComplete && onExitComplete()
}

This means already-exited elements would still be visible until the next render (if there was one)

I looked into adding a test for this case. However, in tests, the use of React.StrictMode created a lot of extra renders which would "fix" this broken state before I could assert against it. Even experimenting with not using React.StrictMode, I still saw more than 1 render when calling rerender and could not get a clean failing test. Let me know if you have any suggestions!

[mattgperry]

Hey, great PR, thanks! Could you commit the test with multiple renders and no StrictMode so I can look into why it's rerendering?

[hluisson]

sure - i've pushed the test up! (this test does not fail on main, so it's not reproducing the bug)

Looking at it a bit more, I actually think React Strict Mode is unrelated to the difference in rendering - both the sandboxed app and the tests run in strict mode, with different results.

I adding some console.log statements locally to try to demonstrate with screenshots - one at the top of the "AnimatePresence" component and one in the onExit callback

In my local version of the sandboxed example from the linked issue, I click "toggle" twice in quick succession, and I see the final onExit cb is the last thing executed (this shows the bug: we fail the check !exiting.size that should be calling forceRender() and there isn't a render after the final onExit cb)

when i run the unit test i've just committed (adding console.log("toggle clicked!") above the lines where i call rerender) against main, I see the following:

in the test code specifically, both cbs execute in the same tick so there is a final render that corrects the state (so - the test does not fail when it should)

let me know if that makes sense!

[mattgperry]

Ah yeah that makes sense thanks, I'll take a look!

[mattgperry]

I think I've got it breaking correctly by changing the test to:

        // Remove the second-to-last element from the array
        await act(async () => {
            rerender(<Component visibleKeys={[0]} />)

            await new Promise<void>((resolve) => {
                //  Resolve after all animation is expected to have completed
                setTimeout(() => {
                    resolve()
                }, 1000)
            })
        })

I think the difference between this and the sandbox is in how act flushes effects.

Edit: Ah but this isn't fixed on branch. I'll keep looking.

[mattgperry]

Ok I've got a test that fails on main and passes on this branch.

#2119

Once I've merged that I'll merge this and remove the Jest test. Thanks again!