Bug: All props are reapplied if any prop changes

[RJWadley]

Current Behavior

When one prop changes, it seems that all the props are re-applied, rather than only applying the changed props.

This is most noticeable when an object has event handlers since functions are passed by reference and 'change' on every render. This means any element with event listeners gets all its props reapplied on every render, which is bad news if you're doing any sort of animations in an effect.

Expected Behavior

When a prop changes, only that prop should apply.

Steps to Reproduce

1. Create a component with an initial state defined in JSX. For example, an initial alpha of 0.
2. Animate or change that value with javascript in an effect. For example, animate the alpha to 1.
3. Add an inline event listener.
4. Cause a re-render. I initially found this bug because I had a hover state that changed state, but any re-render works.
5. When the component re-renders, the props will get reset. For example, the alpha would reset to 0.

I've made a simple reproduction here. In this example, the initial alpha of 0.1 is incorrectly re-applied on every render because of the event handler. This causes the sprite to switch between the initial value (0.1) and the value applied in the effect (1.0) endlessly. Comment out the onclick property to see the expected behavior.

Environment

• @pixi/react version: 7.0.3
• pixi.js version: 7.2.1
• React version: 18.2.0
• ReactDOM version: 18.2.0
• Browser & Version: Chrome 111
• OS & Version: MacOS Ventura 13.0.1
• Running Example: https://codesandbox.io/s/event-props-bug-qjng9d?file=/src/App.js

Possible Solution

No response

Additional Information

No response

[maximtwo]

Hey @RJWadley, I'm not one of the authors of this library so take what I have to say with a grain of salt but what you're experiencing is, in my eyes, the expected behavior.

React provides us with a hook for this specific case: useCallback.

const onClick = useCallback(() => {
    console.log('click')
  }, [])
  
  return <Sprite
      ...
      onClick={onClick} />

When you use an inline function as a react prop, that's technically a different reference to a function each time the component re-renders which would cause the Sprite component to re-render.

[RJWadley]

Hi @maximtwo, thanks for your response! Standard react behavior is to only apply props if they've changed. This behavior is intentional and is one of the things that makes react fast. I've tweaked the example I provided earlier to run in standard react. If this were the expected behavior we would see the box flickering between red and green (I'm changing className instead of alpha).

The callback solution you've provided actually works great for simple solutions, but can't save us every time. Imagine, for example, a simple box that fades in, then moves around the screen randomly. When I click the box, I want the camera to center on the box. Here's what that callback would look like:

  const click = useCallback(() => {
    moveCamera(x, y)
  }, [])

But wait, this isn't working at all... Oh, that's right, we're missing some dependencies from our callback! I'll just add those in:

  const click = useCallback(() => {
    moveCamera(x, y)
  }, [x, y])

You've probably figured out why this doesn't work now. If x and y are changing every render, now this callback is too. This means all of our props get reset every time x and y change, including the alpha from that fancy fade-in I mentioned earlier. But also, it's not just callbacks! Anything else can cause this too, including objects and regular variables.

[maximtwo]

I think I'm just flat out wrong about the way react treats functions passed inline via props and great example proving me wrong there. The empty dependencies array passed to useCallback was not an oversight btw ;) but I see what you mean now about how the props are being reapplied on subsequent renders and I'll bow out of that conversation.

One thing I'll add though is that you might be going against the grain here by changing things via refs instead of just props. For something like this I would use (heh) useState to store the alpha (or any other prop) of the sprite and modify that instead of touching the ref directly. Changing props in that way is not slow if that's what you're thinking.

[michalochman]

I can confirm this is indeed a bug and was one of the reasons I decided to base a rewrite of react-pixi-fiber reconciler methods on the implementation of react-dom

I amended the provided example to show that the bug is not happening when using react-pixi-fiber: https://codesandbox.io/s/event-props-bug-forked-ij2qix?file=/src/App.js

Since we decided @pixi/react to be the main successor, I suggest we port that implementation here, so we don't have to reinvent the wheel. Alternatively we can look at what react-dom is doing currently and reimplement that if it's different.

[baseten]

@maximtwo @RJWadley - I believe you're both right (sort of).

@maximtwo what you're describing is exactly how React works in user-land: A render will always happen on any prop change, unless you're specifically preventing it via a custom props comparison function in React.memo or using the old class based shouldComponentUpdate API. Using any inline non basic prop value (ie. object, array or function) will always trigger a rerender as these will be new instances by reference on each render and that is exactly what useCallback and useMemo were designed to handle.

@RJWadley your example is exactly how I would expect it to behave, but I agree that the vanilla React version does work, albeit I would suggest by accident rather than design.

Currently react-pixi applies props to the underlying PIXI instance irrespective of whether they've changed, which does seem weird to me and I'm unsure of why this was the original design decision:

https://github.com/pixijs/pixi-react/blob/master/packages/react/src/utils/props.js#L178-L207

Your vanilla React version works because when the div rerenders due to the callback reference change, the old and new props are compared and because the className prop hasn't changed between renders (it's red both times) it isn't reapplied to the div. The fact it remains green is arguably not something to rely on, effectively at this point React's virtual DOM is out of sync with the real DOM, because you've updated the real DOM behind React's back 😄. Generally this is why doing this sort of access in a useEffect is discouraged, but of course there's cases where it's necessary.

Going forward the "fix" would seem to be to avoid applying props unless they've changed, I don't know if this might have any other repercussions though.

[michalochman]

@baseten for sanity reasons I think it would make the most sense if react-pixi behave similarly to react-dom as it would make working with both PIXI and DOM more predictable.

Currently react-pixi applies props to the underlying PIXI instance irrespective of whether they've changed, which does seem weird to me and I'm unsure of why this was the original design decision:

The actual origin of this logic you can find in the original react-pixi package (for React before fiber was a thing), see:
https://github.com/Izzimach/react-pixi/blob/ccfad3df30ebecfef52171dac6ce239479b39bde/src/ReactPIXI.js#L213-L232

When I wrote the first fiber reconciler I was going for maximum compatibility, so I wouldn't have to do a complete rewrite of our applications using react-pixi when switching to react-pixi-fiber. At that point I didn't really try to understand decisions made in the original react-pixi, I was merely trying to make my applications work in React 16.

Later we discovered many issues with updating the scene tree, so I decided to look at what react-dom was doing and rewrite react-pixi-fiber to do it the same way, hence the changes introduced here:
michalochman/react-pixi-fiber#43
(the code comments there refer to equivalent functions found in react-dom).

The change meant that only props that changed were applied. If you have look at the example sandbox I linked in my comment above you'll see that the behavior is comparable to react-dom example provided by @RJWadley.