New "rerender children" behavior in 2.2.0 causes undesired UX due to image re-render time

[jarredt]

The new behavior introduced in 2.2.0 that automatically re-renders children as the index changes is causing undesired UX as children are swiped through. In my case, I have a large image at the top of each child, and so as the swipe begins the images are there, then as the swipe is released and moves to the next card, the images briefly disappear and the text below pops up into the space the image occupied. By the time the cards settle, the images have appeared again and the text is back in place.

I may be able to mitigate some of the adverse effect by putting the image in a container that holds the space, but that won't stop the image itself from flickering in and out.

I would advocate for adding a prop to disable this force re-render behavior. The behavior is a little bit of a heavy solution to some of the issues that it seeks to solve. I believe those issues were mostly wanting to optionally rerender children (or an individual child) through changes to their props, for example. But this solution is forcing a re-render with every swipe, even if a re-render is not desired or is potentially problematic.

[jarredt]

Alternatively, if there is another approach to accomplishing the forced re-render besides manipulating the key, that may also help avoid the undesired UX. Changing the key causes a full teardown and skips the diffing evaluation, which is likely the cause of the flicker. It's less of a "re-render" than it is a full unmount and re-mount.

[kopax-polyconseil]

@jarredt the fact that the card does not rerender was an undesired behavior reported in many issues. If you were relying on it to prevent you rerender, then it's wrong.

I suggest that you manually prevent the re-rendering so you can keep upgrading the library, see https://stackoverflow.com/a/54947786

[jarredt]

Thanks for the reply, @kopax-polyconseil, and for working to solve this problem. Definitely understand the child re-render behavior was a pain point and am grateful for your work to address it.

Looking at the issues cited in the PR (#22 #37 #50 #51), it seems the concerns cited are that the children were not re-rendering based on external prop or state changes -- not that they didn't re-render as the cards entered/left the active state. While I understand that the solution to "re-render" on card transition may fix the symptoms reported, I did think it was worth pointing out the negative side effects.

I would argue that it's not an unreasonable expectation that a component will not re-render simply because its position is being positionally translated in and out of the viewport. But more than that: the implemented solution isn't technically a "re-render" of the same node -- because it's doing a key change, the existing node is fully torn down in favor of a new node (ref article). In addition to the performance impact of redrawing the entire child again, I believe it might also break any refs that the parent might be holding to the child components (I have not tested this part).

Anyway -- I will give the memoization a try, although I'm not sure that'll help given that the entire child node is being torn down and reinstantiated. I can stay on the older version for now if that doesn't work, but I do think it's worth further considering the costs of the approach.

[jarredt]

Key change on child may also cause that child to lose its internal state: https://reactjs.org/docs/reconciliation.html#tradeoffs.

[kopax]

Hi @jarredt.

I believe it might also break any refs that the parent might be holding to the child components (I have not tested this part).

We are passing in our app the useRef<Swiper>(null) to <Swiper /> and each child, the ref work perfectly, you can see our implementation here, as you can see, each child we create have access to the swiperRef and we use it to control the activeIndex. It work very well.

it seems the concerns cited are that the children were not re-rendering based on external prop or state changes

It was not rerendering because we create child statically, using a key change will force the rerendering, but we only refresh it when it become active. I don't see a situation where we want other card to be rendered when they are out of the viewport, and they are, they just are rendered on init and keep their state, focusing will rerender and that seems to be a nice moment to trigger a full rerendering, the state will change, but you have control on it's behavior when it is re-rendered, you should not rely on it in non focused card anyway, especially if you are updating thing in it and expect to keep them in their latest state, instead, use props and some context or reducer.

[kopax]

In any case, if you have a better idea for optimizing the rendering as this is public repository, I am up to test it. The implementation I did seems to be a quick but clean way of doing the re-rendering when necessary and it's infinitely much more flexible than before.

[jarredt]

Thanks. I disagree with some of your points, but that's ok! 😎

Do you happen to know why we need to render the children statically to begin with? Why can't props.children be rendered directly?

On Sat, Feb 13, 2021, 1:41 AM Dimitri KOPRIWA notifications@github.com wrote:

In any case, if you have a better idea for optimizing the rendering as this is public repository, I am up to test it. The implementation I did seems to be a quick but clean way of doing the re-rendering when necessary and it's infinitely much more flexible than before.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/reactrondev/react-native-web-swiper/issues/65#issuecomment-778591209, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD7LLP76IXM2TSIGOYZ6UXLS6ZCMLANCNFSM4XNMYXCA .

[kopax]

I have no clue. Did you had a look at it ? Perhaps you can try to improve this with some forceUpdate on each node instead of key change ?

[c0m1t]

It was not rerendering because we create child statically, using a key change will force the rerendering, but we only refresh it when it become active.

@kopax I think as @jarredt mentioned you can do it when user asks the current component changes key and receive an option for that.

{cloneElement(el, { key: i - this.getActiveIndex() ? i : -1, activeIndex: this.getActiveIndex(), index: i })}

Correct me if I'm wrong, you clone each element and add key, activeIndex and index props. Let's say we have the following instead:

{cloneElement(el, {activeIndex: this.getActiveIndex(), index: i })}

Swiping makes the activeIndex to change, so shouldn't the component be rerendered? What happens if the key attribute is removed from the new props of cloneElement?

What will happen if you move these two lines inside of the render function? Does that help children to get updated in each render?

children = (() => React.Children.toArray(this.props.children))();
count = (() => this.children.length)();

And you can always use context instead of injecting new props using cloneElement.

[evilripper]

I tried to make a little gallery of images and I have the same issues. the image disappears and appears in the browser too.

Is there a way to disable re-rendering in a child component?

[kopax-polyconseil]

yes there is, you can patch package and remove the introduced feature in charge of forcing the rerendering, or do a pull request to add this as a feature

[jarredt]

I've introduced a PR to address this issue: https://github.com/reactrondev/react-native-web-swiper/pull/74.

[jarvisluong]

Thanks for the discussion! New version 2.2.2 has been published for this change.