Closed swissspidy closed 2 years ago
Ahh great find. I'll look into this
Alright, so did a little recon on this today to try and figure out some possible solutions. Want to do a little brain dump so we don't lose any context while I'm out on vacation.
React's diffing algorithm with our animation patterns, look to be the primary culprit. Basically if the parent is different, react will destroy the old tree and build up a new one (we're essentially opting out of one of the heuristics react relies on with this pattern): https://reactjs.org/docs/reconciliation.html#elements-of-different-types https://github.com/google/web-stories-wp/blob/e0af2f160f4141e77179253b5d10b38b3c30735f/packages/animation/src/components/WAAPIWrapper.js#L28-L51
The WAAPI implementation used to only hold pure data in provider state and not actually hold components. I believe we originally altered it to hold components just to mirror our AMP implementation.
I see no reason why we can't revert the underlying generated WAAPI animations to just be pure data and keep all the markup in our WAAPIWrapper
component. I believe we can then structure this in a way where react doesn't regenerate underlying DOM nodes on every animation update, but instead just updates the props.
To update the provider state to only hold pure data for WAAPI animations, we'll need to go through our animation parts and effects to essentially only return these attributes for the WAAPIAnimation property:
{
timings,
keyframes,
useClippingContainer,
targetLeafElement,
}
Most of this should be taken care of by simply updating simpleAnimation.js
, however, there are some animation effects that compose their animations by using nested react components ie flyIn
, rotateIn
, etc. For these we'll need to go through and recompose the animation through keyframes (kind of like how we do in backgroundPanAndZoom
)
Lastly we'll need to update WAAPIWrapper
to expect WAAPIAnimation
as pure data and render markup accordingly that doesn't violate the react diffing heuristics.
Alternative Approaches
Originally I was trying to think of a way to opt out of react recreating a new tree of components when the rendered children
hasn't changed. Seems to be a common problem with things like drag n drop libraries where they may move an expensive component to a new container, and don't want react to regenerate that expensive component when nothing has changed but its parent.
The term for this scenario is called reparenting
:
https://github.com/facebook/react/issues/3965
This thread has two propositions to get around this.
Conclusion Ultimately I think it's better to just go back and fix the underlying patterns to be more compatible with reacts current reconciliation algorithm vs trying to further opt out of them because we're doing something antithetical to idiomatic react.
Tested against https://stories-qa-wordpress-amp.pantheonsite.io/wp-admin, using PR #10024.
Added videos to the canvas and moved them, changing their position. No new requests found on Network > Media tab.
Bug Description
This finding was kindly shared with us by rtCamp engineers:
Issue: Any element wrapped by
StoryAnimation.WAAPIWrapper
/StoryAnimation.Provider
get's re-rendered whenever an element gets moved to another place. This also causes videos or images to be loaded again due to the re-renders, resulting in tons of HTTP requests.Cause: currently, all the animation properties of display elements are being stored in a
Map
, so whenever the (x, y) position of an element changes, thatMap
gets updated and all of those elements depending on animations get re-rendered.Relevant code:
https://github.com/google/web-stories-wp/blob/31776678fb986ad1f11461ca9febf900ec4f7c3d/packages/animation/src/components/provider.js#L81-L118
Expected Behaviour
No unexpected re-renders
Steps to Reproduce
Check renders in React dev tools, see also video below showing the HTTP requests
Screenshots
https://share.vidyard.com/watch/PTXDn2R1L7XqPyX57Y3tg4?
Additional Context