pmndrs / react-spring

✌️ A spring physics based React animation library
http://www.react-spring.dev/
MIT License
28.16k stars 1.19k forks source link

animated does not work as expected on components also wrapped with forwardRef #1639

Closed gpoole closed 2 years ago

gpoole commented 3 years ago

🐛 Bug Report

Using forwardRef on a component seems to stop animated from updating the component in response to the props received from useSpring.

To Reproduce

The following usage causes the issue described:

const AnimatedNotWorking = animated(
  forwardRef(({ value }, ref) => {
    return <div ref={ref}>{value}</div>;
  })
);

Expected behavior

The component wrapped in animated should be re-rendering in reaction to changing animated props, which it does if forwardRef isn't used, for example the following works as expected:

const AnimatedWorking = animated(
  ({ value }) => {
    return <div>{value}</div>;
  }
);

Link to repro (highly encouraged)

https://codesandbox.io/s/charming-mestorf-hptid?file=/src/App.js

In the sample, AnimatedWorking is working as expected and re-rendering continuously as the animated value changes, AnimatedNotWorking is only rendering once.

Environment

kindoflew commented 3 years ago

After a little digging I found this article explaining how forwardRef and HOCs can behave a little weird when used together (animated being an HOC).

I hobbled together a version of your example using the solution in that article and it seems to work (the background-color is set to 'pink' using the ref in App.js and the value is still animated correctly).

So, I think this is intended (if unexpected) behavior for React.

gpoole commented 3 years ago

Thanks for looking into it @kindoflew. The article is describing workaround for HOCs that don't forward refs so the ref is being dropped, but as the article mentions:

We could update our HOC to pass on refs, but we might want it to be used with components that don't accept refs. So what can we do?

And in this case animated is actually correctly forwarding the ref to the wrapped component, so refs are working as expected with animated components. The issue I'm reporting is different though. In my repro the working component constantly renders with the changing spring value, whereas the broken one renders once on mount and stops, which is the problem.

The sample you've posted does fix the issue by avoiding using forwardRef on the wrapped component itself so it's a good solution for avoiding the bug, but it's breaking out of the normal ref passing process for React and it's a workaround so I think it would be worthwhile addressing the underlying problem.

Just to add a bit of detail: the broken component does render in response to other prop updates, it just doesn't seem to be updated at all by the animated spring properties changing. I've also noticed in my app that if the component happens to be re-rendered by other props changing it renders with a snapshot of where the spring properties were at at the time.

kindoflew commented 3 years ago

More digging!

So, if you inspect the div in dev tools in your example, you can see that value is added to it as an attribute (sometimes it doesn't always update there either, which is why I didn't notice it at first. I think it's a codesandbox thing -- refreshing a few times got it to work for me. I made a local version of your example and can see it every time). So value is still updating, it's just not updating when used directly in the view.

I think what is happening is that since all animated elements are using forwardRef themselves, animated is hijacking your ref and then spreading the props onto the element as attributes (happens somewhere around here). So, while you can get those snapshots of value when other things cause a re-render, react-spring isn't re-rendering because, as far as it knows, it's doing its job by updating the attributes. (I messed around with adding some checks for Component.$$typeof !== 'REACT_FORWARD_REF_TYPE' where this occurs, and it did end up with your expected behavior, but it also caused most animated tests to fail.)

However! If we refactor slightly we can get working refs, and access to value in the view. That still might not be the solution you're looking for, but as far as I can tell (I'm still learning the code base) animated is relying on this behavior for the other stuff it does.

I could also be totally wrong about that, but it's beyond my current knowledge of react-spring's guts.

gpoole commented 3 years ago

Thanks for the detailed follow up and research @kindoflew. Yes I think you're exactly right there, it looks like withAnimated applies updates to the thing it's wrapped in one of two ways: 1. updating a DOM element's attributes or 2. forcing an update. My sample component is forwarding its ref to a DOM element so withAnimated is treating the component itself like it was a DOM element and going with 1.

Ideally I think it would be good to have a way to opt-out of this optimisation since I think my use case is reasonable. I wonder could there be something like a flag for the HOC to always force updates so that I can make my intention explicit in the code? I think currently withAnimated is inferring intention from the refs it gets and with forwarded refs I think that's not always going to be correct, so I wonder if there's a better way?

To explain my use case, I have a 3D SVG cube I'm rendering a bit like this:

const Cube = forwardRef(({ size, rotation }, ref) => {
  const paths = computeCubePaths(size, rotation);
  return (
    <svg ref={ref}>{paths}</svg>
  );
});

I want to use react-spring to drive the size and rotation props, so I need the component to be re-rendered to make these updates. I'm using the forwarded ref to measure the DOM node's position and dimensions.

In terms of solutions to my problem I'm passing the ref in a regular property like in the solution you suggested, but I think this behaviour isn't intuitive (I lost a bit of time figuring out the problem) and the fix we're talking about is a workaround, so I thought it was worth seeing if a more permanent fix was possible or if it could be documented.

kindoflew commented 3 years ago

Finding the issue was one thing, but fixing it is probably beyond me right now (or at least it would take a while :sweat_smile:).

However, for the case in your Cube example -- would it be possible to just return <a.svg ref={ref}>{paths}</a.svg> instead of wrapping the whole component in animated?

gpoole commented 3 years ago

No unfortunately that wouldn't work, I'm relying on the size and rotation props to the Cube component being animated to update the paths that make up the faces of the cube, the svg tag itself isn't changing.

fast-reflexes commented 2 years ago

I think react-spring is intended to work exactly like what you see.

react-spring does its work outside of React in order to provide smooth transitions. This requires updating elements in the DOM via refs, instead of rerendering them with React. This is why the recommended way is to use

const [spring, api] = useSpring(() => ({ value: <VALUE> }))

instead of

const spring = useSpring({ value: <VALUE> })

(in the top case, as long as the parent doesn't rerender, your child element can be updated by simply using the api which does not require you to rerender the animated component; alas, the spring object is the same even after updating it via the api).

When you use the <animated.XXX> components, react-spring takes care of this for you. Because these native elements are interacted with in the exact same way as native DOM elements are interacted with, props and content added to these are guaranteed to follow the same API as the native DOM elements themselves which makes updates via refs outside of React a possible job for react-spring. When you add your own custom components, you have to honor similar principles:

Log in the console whenever your component rerenders to keep track of this behaviour and see for yourself.

If we go back to your example

const AnimatedNotWorking = animated(
    forwardRef(({ value }, ref) => {
        return <div ref={ref}>{value}</div>;
    })
);

This component suggests a use like

<AnimatedNotWorking value={<SPRINGVALUE>} />

Now, since a div does not have a property value, there is no way for react-spring to know how to update this div via the ref. It will simply set the property value of the div to the updated value in the SpringValue which will have no effect at all. When you DON'T give react-spring a chance to set a ref on your custom component, it rerenders on every animation frame and then it works but is sub-optimal from a performance view. So either you accept a drop in performance and don't use a ref in your component, or you make it possible to set a ref and adhere to the restrictions.

You have to see the resulting component wrapped in animated as a static element with props provided in a way so that react-spring can figure out how to update the DOM element. In the above case, this implies rewriting the call to your function to

<AnimatedNotWorking>{<SPRINGVALUE>}</AnimatedNotWorking>

Now, the only time React renders this, it will add the SpringValue as the property children to the wrapped component and animated knows how to update this special property. However, other properties will simply be added to the native DOM element in question which often does not have the intended effect.

Here is a sandbox with this example where you can verify all of this behaviour (when it works, when it sets the value property of the div and when it rerenders on every animation frame): sandbox

If you show exactly what computeCubePaths return and how you use your component, I'm sure I can help you find a way which makes it work like you want. Possible solutions for you is to add the path elements and supply SpringValues as props (and thus NOT use animated to wrap your component and instead use animated native elements inside your component).

const Cube = ({ size, rotation }) => {
    const paths = computeCubePaths(size, rotation);
        return (
            <svg>
                <animated.path { ...<SPRING VALUES> } />
                <animated.path { ...<SPRING VALUES> } />    
            </svg>
        );
});

If you only update a single native element, then you should create such an element in your wrapped component and pass updated props (which have the same names as the native properties of the element to which you add the ref) right to it.

const Cube = animated(
    forwardRef(({ size, rotation }, ref) => {
        return (
            <svg>
                <path ref={ref} size={size} rotation={rotation} />
            </svg>
        );
    })
)

You should also think about that when you're adding children as an animatable content, I find it VERY hard to believe that, everything else aside, it would render anything else than the actual TEXT that you add there. react-spring is not a magician and it can add DOM elements and animate them from an input text.

All in all, I think a good thing to think about is that react-spring animates by constantly updating properties / styles on elements to create the exact effect intended. This as opposed to using CSS transition to accomplish the same. Apart from that, there is no magic going on and react-springs biggest challenge is to do this inside React without making it too slow.

Hope this helps, otherwise reach out! Good luck!

gpoole commented 2 years ago

Thanks for the detailed follow up @fast-reflexes. I think the solution of using animated.path is a good one for my use case and would be cleaner and probably more efficient than what I had. I don't think it solves every possible use case, though. For example you may wish to pass animated props to a third-party component that you don't have access to the internals of.

I agree that as you say this behaviour is the result of how react-spring internally handles refs, however it's still surprising behaviour to a user of the library that combining forwardRef with animated silently causes animations to stop working. This is the reason I've left the issue open, not because I'm looking for support on it, although I think your suggestion is a good one and you're right that there are uses of animated that could be done other ways.

I think probably this is not a common problem and also not one that's easy to fix, so I've opened a PR (#1734) to add some documentation about it.

fast-reflexes commented 2 years ago

Good solution! I think that updating the docs was the perfect resolution to this ticket. As a side note, I never understood if you wanted to pass a ref for the sake of react-spring or not.. Now I realize you wanted to pass a ref for other reasons than react-spring and then I totally agree that the resulting behaviour is a bit unexpected and requires a lot of internal knowledge of react-spring. In a longer perspective, perhaps a flag could be set to indicate whether to use the forwarded ref for updates or not but in the meantime, I think the suggested workaround is very appropriate. Good job!