Skip to content

Remove key change logic #74

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

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

jarredt
Copy link
Contributor

@jarredt jarredt commented Dec 13, 2021

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.

@oxyii oxyii requested a review from jarvisluong December 13, 2021 18:54
Copy link
Contributor

@jarvisluong jarvisluong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jarvisluong
Copy link
Contributor

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

@jarvisluong jarvisluong merged commit 0bed7e6 into reactrondev:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants