search

reactrondev / react-native-web-swiper

Swiper-Slider for React-Native and React-Native-Web
213 stars 55 forks source link

Remove key change logic #74

Closed jarredt closed 2 years ago

jarredt commented 2 years ago

PR #57 introduced some changes to ensure that children re-render when the Swiper is swiped. Specifically, it made use of React.cloneElement to provide each child with two new props (the current activeIndex which changes on swipe, as well as the child's specific index). It also sets a key prop on the children that is changed from the index to -1 for the child that becomes to currently active child (and from -1 back to the index for the child that is ceasing to be the active child).

This PR removes the dynamic key logic. Issues #65 and #67 discuss the issues/concerns with changing the key on children on swipe. Specifically, changing the key on a child does not cause a "re-render" -- it actually causes React to fully tear down and remount the component, skipping React's diff inspection logic.

As the React docs call out:

Keys should be stable, predictable, and unique. Unstable keys (like those produced by Math.random()) will cause many component instances and DOM nodes to be unnecessarily recreated, which can cause performance degradation and lost state in child components.

In particular, changing the key like this causes issues with children that contain images that may take some time to reinitialize (causing an undesired flickering effect). It also means the children will lose any internal state they may be maintaining.

Passing the activeIndex prop (whose values changes on swipe) is sufficient to re-render the children (I verified this locally using a patched version of the library in my project with this PR's change applied). If components further down in the child tree need to re-render on swipe, then it should be the child's responsibility to incorporate logic to ensure that happens (e.g. by passing the activeIndex down into those sub-components). Using the child key change to nuke the entire child is too aggressive.

That said -- if the key behavior is truly required/desired, I think it should be opt-in behavior via a new prop (e.g. dangerouslyRerenderChildren) on Swiper, rather than have it be default behavior.

jarvisluong commented 2 years ago

Hi! Thanks for the discussion and PR! I will publish a new version soon.