useSprings does not properly clear controllers when length changes in React.StrictMode

[tarngerine]

🐛 Bug Report

When changing the length provided to useSprings in combination with a propsFn, the internal SpringRef does not properly dispose of the old refs when the array changes.

To Reproduce

See CSB

Expected behavior

See CSB, while the springs are correctly oscillating between lengths of 8 and 9, the api.current controllers always increase.

Link to repro (highly encouraged)

https://codesandbox.io/s/elastic-hofstadter-m48zc?file=/src/UserTiles.jsx

Environment

• react-spring v9.2.1
• react v17.0.2

[joshuaellis]

This looks like it was brought about in this commit – 9176c5a Might have been when I used useRef to assign the Controllers.

This fix shouldn't be too hard, if youre interested in giving it a go and contributing! ⭐

[tarngerine]

might have some time this week, thanks!

[tarngerine]

hm, i added some tests to useSprings.test.tsx but it seems to be passing locally. any thoughts on this?

[tarngerine]

oh hm, if im reading it correctly, these tests create a new useSpring hook instance every time update is called?

[joshuaellis]

No it doesn't because you don't destroy the spring when you update the props because of internal state etc.

calls the props function once per new spring is an example of this

[tarngerine]

ah, i see...

so without making any changes to the useSprings hook, i downloaded the CSB as a zip and added it to the demo page and it works fine

i did have to change the package import to @react-spring/web

any clues on how this difference in behavior on prod 9.2.1 vs local monorepo might happen?

[joshuaellis]

It's StrictMode. Add it to ReactDOM.render and the bug is there. see: https://codesandbox.io/s/beautiful-leaf-k2xcc

[joshuaellis]

Might be something to do with this – https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects specifically:

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following functions:

• Class component constructor, render, and shouldComponentUpdate methods
• Class component static getDerivedStateFromProps method
• Function component bodies
• State updater functions (the first argument to setState)
• Functions passed to useState, useMemo, or useReducer

[tarngerine]

interesting re: removing strictmode fixing the bug in CSB. i can dig into that too

however, my original point that i cannot repro in the cloned react-spring monorepo still stands — i had strictmode on there, too, and it works "fine". i added the CSB source code to /demo

ideally we could test for this so i'm just trying to get a local repro, bc i have no idea how i can fix this in the CSB environment as it's referencing an npm package

[joshuaellis]

I think I built the package and changed the web alias from src/index to dist/NAME.esm.js. This would emulate the npm package afaik...

[tarngerine]

sorry i dropped off, life happend!

so to make sure i undersatnd — you built this repo locally and moved the dist JS files into the CSB (or a cloned CSB locally). i havent done much aliasing before — how does that work? do you just change the import statement to be import X from 'dist/NAME.esm.js'?

[joshuaellis]

in /demo there's vite.config.js, it looks like this:

import path from 'path'
import { defineConfig } from 'vite'
import reactRefresh from '@vitejs/plugin-react-refresh'

export default defineConfig({
  resolve: {
    alias: {
      '@react-spring/web': path.resolve('../targets/web/src/index.ts'),
      '@react-spring/parallax': path.resolve(
        '../packages/parallax/src/index.tsx'
      ),
      '@react-spring/three': path.resolve('../targets/three/src/index.ts'),
    },
  },
  plugins: [reactRefresh()],
})

This is where we alias the packages to the src code. Running yarn dev then lets you edit the src code and see those updates in the demos. But you could run yarn build and change that alias to the dist path of web for example.

[tarngerine]

got a few hours to dig into this

Proper repro steps

• Add <React.StrictMode> to /demos/index.tsx — I was adding StrictMode to the wrong place
• Relinking / building is not necessary!
• Run yarn dev any time you update the hooks to see changes

Findings

When in StrictMode, the "Cleanup any unused controllers" step does not run because oldCtrls is always empty inside the useLayoutEffect

I can't quite figure out why oldCtrls is being effected non-deterministically due to the double invoking useLayoutEffect, but ctrls is not.

I've even tried to replace the oldCtrls definition with:

• a useRef, and updating it inside the [length] useMemo

• state.ctrls.slice(length, prevLength)

the oldCtrls inside useLayoutEffect is always empty

Potential fix

The one way I could fix this issue was to move the entire "Cleanup any used controllers" function into the length useMemo, but this seems like dangerously moving logic that may have other effects. The tests are passing, though.

Any guidance you have here would be great — if I should move ahead with this potential fix or look elsewhere.

[joshuaellis]

Maybe make a PR? It's easier to review that way and our CI will start building some sandboxes for us at the same time so we can review a bit better.

[tarngerine]

@joshuaellis done! https://github.com/pmndrs/react-spring/pull/1597/files#diff-e1288b0efcce202f0575295aec86c6f33c5d81adac9751131dc5d2b08e40e73aR136