Skip to content

onRest callback within useSpring cannot access updated component state #660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bmcmahen opened this issue May 4, 2019 · 7 comments
Closed
Labels
kind: bug Something isn't working

Comments

@bmcmahen
Copy link

bmcmahen commented May 4, 2019

This isn't really a bug per-se since it's kinda how closures work, but let's say you have a useSpring like this:

const [isOpen, setIsOpen] = React.useState(false);
 
 const [{ xy }, setSpring] = useSpring(() => {
    const { x, y } = getDefaultPositions(isOpen, position);
    return {
      xy: [x, y],
      config: animationConfig,
      onRest: ya => {
        console.log(isOpen);
      }
    };
  });

From my experience, isOpen will always return false even if its value has changed to true within the component. Ideally we could access updated state here.

You can see this in action on this codesandbox. Open the console and try dragging the dot around.

In my experience, you can fix this sort of thing by doing something like this:

function useSpring(fns) {
  const callbacks = React.useRef(fns);

 React.useEffect(() => {
   callbacks.current = fns;
 }, [fns])

 // and then make calls to callback.current.onRest... 
}

I've taken a quick look at the source, but feel a bit like a deer in headlights. Not sure how easy this would be to fix for someone more knowledgeable of the codebase.

@bmcmahen bmcmahen added the kind: bug Something isn't working label May 4, 2019
@bmcmahen
Copy link
Author

bmcmahen commented May 4, 2019

I've found a solution on my end. Refs to the rescue!

const [isOpen, setIsOpen] = React.useState(false);
const isOpenRef = React.useRef(isOpen);

React.useEffect(() => {
  isOpenRef.current = isOpen;
}, [isOpen])
 
 const [{ xy }, setSpring] = useSpring(() => {
    const { x, y } = getDefaultPositions(isOpen, position);
    return {
      xy: [x, y],
      config: animationConfig,
      onRest: ya => {
        console.log(isOpenRef.current);
      }
    };
  });

I still wonder if this could be made easier.

@aleclarson

This comment has been minimized.

@aleclarson
Copy link
Contributor

aleclarson commented Jul 2, 2019

@satya164 recently pointed out here that my solution is incorrect. (Thanks Satya!)

Here's another workaround that should work as expected:

const [isOpen, setIsOpen] = React.useState(false)
const [props, update] = useSpring(() => ({ ... }))
React.useEffect(() => {
  update({
    onRest() {
      console.log('isOpen:', isOpen)
    }
  })
}, [isOpen])

Once v9 is released, you'll be able to use a "dependencies array" just like useEffect:

const [isOpen, setIsOpen] = React.useState(false)
const [props, update] = useSpring(() => ({
  /* ...initialProps */
  onRest() {
    console.log('isOpen:', isOpen)
  }
}), [isOpen])

Note: If your initial props are expensive to compute (which they usually are if you're passing a function to useSpring), you'll want to wrap them in a useMemo call like this:

const [isOpen, setIsOpen] = React.useState(false)
const initialProps = React.useMemo(() => ({ ... }), [])
const [props, update] = useSpring(() => ({
  ...initialProps,
  onRest() {
    console.log('isOpen:', isOpen)
  }
}), [isOpen])

Once again, any ideas to make this easier are welcome in this thread.

@aleclarson
Copy link
Contributor

aleclarson commented Jul 2, 2019

Most of the time, it's premature optimization to pass a function to useSpring. For your own sanity, just pass an object.

const [isOpen, setIsOpen] = React.useState(false)
const [props, update] = useSpring({
  ...initialProps,
  onRest() {
    console.log('isOpen:', isOpen)
  }
}, [isOpen])

Note: This is the v9 API that isn't released yet (not even in the beta). See here: #670

@bmcmahen
Copy link
Author

bmcmahen commented Jul 5, 2019

Looking forward to the new API - looks great. fwiw, i've consistently been using my ref solution that I posted above, which seems to work.

@jonisapp
Copy link

Most of the time, it's premature optimization to pass a function to useSpring. For your own sanity, just pass an object.

const [isOpen, setIsOpen] = React.useState(false)
const [props, update] = useSpring({
  ...initialProps,
  onRest() {
    console.log('isOpen:', isOpen)
  }
}, [isOpen])

Note: This is the v9 API that isn't released yet (not even in the beta). See here: #670

Passing a function rather than an object allows to directly apply the animation on the component, thus avoiding a useless re-render of it and all its children. So it's a costless optimization which could make a big difference (as if the component you are animating does contain a large tree).

@jonisapp
Copy link

jonisapp commented Jul 29, 2020

The best solution I have found is to put onRest in the setter function. Like this :

const [ isOpen, setIsOpen ] = useState(false);
const [ spring, setSpring ] = useSpring(() => ({
  config: { ... }
}));

setSpring({ ...props, onRest() { console.log(isOpen) } })

From my code :

const [ taskEditMode, setTaskEditMode ] = useState(false);
const [ taskPannelIsOpen, setTaskPannelIsOpen ] = useState(false);
const [ taskEditSpring, setTaskEditSpring ] = useSpring(() => ({
  config: { duration: 1000 }
}));

setTaskEditSpring({ height: taskEditMode ? 5 * yUnitLength : yUnitLength, onRest: () => {
  setTaskPannelIsOpen(taskEditMode) }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants