[bug]: React 18 StrictMode logic seems incorrect

[billyjanitsch]

Which react-spring target are you using?

• @react-spring/web
• @react-spring/three
• @react-spring/native
• @react-spring/konva
• @react-spring/zdog

What version of react-spring are you using?

9.4.5

What's Wrong?

This comment (and the associated behavior) introduced in #1875 doesn't seem correct:

react-spring/packages/core/src/hooks/useTransition.tsx

Lines 106 to 113 in 28ef2d1

	/**
	* This _should_ only run in `StrictMode` where everything
	* is destroyed and remounted, because the enter animation
	* was most likely cancelled we run it again on initial mount.
	*
	* This does nothing when `StrictMode` isn't enabled,
	* because usedTransitions on mount is typically null.
	*/

Specifically, "usedTransitions on mount is typically null" is not true. By the time this effect runs, transitions has been assigned to usedTransitions.current:

react-spring/packages/core/src/hooks/useTransition.tsx

Lines 101 to 103 in 28ef2d1

	useLayoutEffect(() => {
	usedTransitions.current = transitions
	})

And populated:

react-spring/packages/core/src/hooks/useTransition.tsx

Lines 163 to 165 in 28ef2d1

	each(items, (item, i) => {
	if (!transitions[i]) {
	transitions[i] = {

So usedTransitions is never null at this point, and this code block always runs on mount with a full array of transitions, regardless of whether StrictMode is enabled:

react-spring/packages/core/src/hooks/useTransition.tsx

Lines 114 to 120 in 28ef2d1

	each(usedTransitions.current!, t => {
	t.ctrl.ref?.add(t.ctrl)
	const change = changes.get(t)
	if (change) {
	t.ctrl.start(change.payload)
	}
	})

To Reproduce

I unfortunately don't have a good user-facing reproduction yet. For context, upgrading (only) @react-spring/web from 9.4.4 to 9.4.5 broke several internal tests and this seemed to be the underlying cause. I confirmed that manually commenting out the new each(usedTransitions.current, ...) block in 9.4.5 fixes all of those tests. StrictMode is not enabled anywhere in this test suite.

I tried to reduce to a single clear reproduction without much luck. I'll keep trying, but thought it might be useful to report this in the meantime since there seems to be a clear contradiction between the intended and actual behavior, based on the comment.

cc @joshuaellis

Expected Behaviour

See above.

Link to repo

See above.

[joshuaellis]

Hi @billyjanitsch, I'd have to see a working reproduction of the issue you're facing to investigate this thoroughly, I saw no issues in our test suite or our demos that we have.

[billyjanitsch]

@joshuaellis does #1891 help? It's a PR that throws in the block that the comment claims will only run in StrictMode. This shouldn't break any of the tests, because StrictMode is not enabled there, but in fact it breaks most of them. It also breaks the demos, etc. with StrictMode disabled. This shows the contradiction between the comment and the behavior.

The bottom line is that 9.4.4 -> 9.4.5 adds extra calls to t.ctrl.start() and t.ctrl.ref?.add() regardless of StrictMode on/off. I don't have enough of an understanding of the library internals to understand the implications of the extra calls, so it's hard for me to put together a user-facing repro. I'll keep trying, though. If you agree that these calls are being made when they shouldn't be, perhaps you have an idea of how this behavior might cause downstream problems? (I understand that the current tests/demos are working but it seems that this is somewhat by accident.)

Alternatively, I could PR a different approach to ensuring that the unmount step only runs once even with StrictMode on. It seems like this was roughly the intention of the change in #1875.

[joshuaellis]

Okay so it sounds like you've taken my comments very literally and it turns out they probably could have been better written.

  useOnce(() => {
    each(usedTransitions.current!, t => {
      t.ctrl.ref?.add(t.ctrl)
      const change = changes.get(t)
      if (change) {
        t.ctrl.start(change.payload)
      }
    })
  })

The code above should only make an impact when called in StrictMode you can't not have something run because StrictMode is on / off... So what i'm trying to deduce is what your issue is, how above is complicit in your issue so we can understand the problem and go from there to find the right solution.

If you can provide a reproduction of the issue or perhaps a test where it fails we can discuss it better.

[billyjanitsch]

So what i'm trying to deduce is what your issue is, how above is complicit in your issue so we can understand the problem and go from there to find the right solution.

Yeah, I totally understand that it will be easier to diagnose with a user-facing failure. I'm working on it. 🙂 The internal failing tests are several layers of abstraction deep and I'm having trouble reproducing in simpler scenarios. I'll update when I can isolate it to a simple, runnable example.

(Roughly, the problems seem to occur in something like the following pattern: useTransition([true]) -> useTransition([false]) -> useTransition([true]) -> unmount. WIP, might be wrong about this.)

it sounds like you've taken my comments very literally and it turns out they probably could have been better written. The code above should only make an impact when called in StrictMode you can't not have something run because StrictMode is on / off.

I think my PR might be misinterpreted. I understood that the code comment isn't literally saying "this code won't run outside of StrictMode." The way that I understood the comment is: "This effect will run twice in StrictMode and once otherwise. On the first run, this code shouldn't have any effect because usedTransitions.current will be null, resulting in a no-op. On the second run, usedTransitions.current will be populated so it will restart any transitions that were torn down in StrictMode cleanup." Is that right? If so, the point of my PR is to demonstrate that it isn't null on the first (or second) run, so the code does have an impact even outside of StrictMode.

I hope this helps clarify why I think this is a real issue, not just nitpicking the way that a comment is worded. I reported in case this observation was enough on its own to see a problem. I'll keep working on a runnable failure if not.

[saseungmin]

I have the same issue.

React ^v18.1.0
Next.js ^v12.1.6
react-spring ^v9.4.4

// next.config.js
const moduleExports = {
  reactStrictMode: true,
  // ...
};

When using the reactStrictMode property as true, an unexpected animation action occurred in the dev environment.
when the reactStrictMode attribute was removed, animation worked normally.

here is example

reactStrictMode: true

2022-05-12.5.27.44.mov

reactStrictMode: false

2022-05-12.5.29.22.mov

[joshuaellis]

@saseungmin you're using the non-working version of react-spring with react@18. You need to use 9.4.5

[saseungmin]

@joshuaellis
Thanks! It works after update.

[joshuaellis]

Hey @billyjanitsch, I haven't forgotten this issue, and I think I finally understand what you mean (sorry for my delay).

Going to look into fixing it, I believe it's most prevalent when you're trying to use a springRef because the animation does not wait for the api to start it.

EDIT: I've also written this failing test that illustrates the problem:

    it('defers rendering until a provided "ref" is called', async () => {
      const ref = SpringRef()

      const props = {
        ref,
        from: { n: 0 },
        enter: { n: 1 },
        leave: { n: 0 },
      }

      update(true, props)

      await global.advanceUntilIdle()

      transition(style => {
        expect(style.n.get()).toEqual(0)
        return null
      })
    })