Skip to content
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

renderItem is call for all data items, even if windowSize is specified #362

Closed
Aure77 opened this issue Feb 5, 2023 · 7 comments · Fixed by #517
Closed

renderItem is call for all data items, even if windowSize is specified #362

Aure77 opened this issue Feb 5, 2023 · 7 comments · Fixed by #517
Assignees
Labels
bug Something isn't working

Comments

@Aure77
Copy link

Aure77 commented Feb 5, 2023

Describe the bug
If you pass a large amount of data to carousel & set windowSize to a small value (ex: 3) to limit the amount of items to be pre-rendered. All elements are still rendered.

To Reproduce

import * as React from 'react';
import {Dimensions, Text, View} from 'react-native';
import Carousel from 'react-native-reanimated-carousel';

function Test() {
  const width = Dimensions.get('window').width;
  return (
    <View style={{flex: 1}}>
      <Carousel
        loop
        width={width}
        height={width / 2}
        autoPlay={false}
        data={[...new Array(16).keys()]}
        windowSize={3}
        scrollAnimationDuration={1000}
        onSnapToItem={index => console.log('current index:', index)}
        renderItem={({index}) => {
          console.log('renderItem', index);
          return (
            <View
              style={{
                flex: 1,
                borderWidth: 1,
                justifyContent: 'center',
              }}>
              <Text style={{textAlign: 'center', fontSize: 30}}>{index}</Text>
            </View>
          );
        }}
      />
    </View>
  );
}

export default Test;

set windowSize to 3 & data with a lot of items. Then add "console.log" in renderItem method. You will see that every item are rendered at first launch.

Expected behavior
Only 3 items should rendered on launch & then rendered on snap.

Versions (please complete the following information):

  • react: v18.0.0
  • react-native: v0.69.5
  • react-native-reanimated: v2.14.4
  • react-native-reanimated-carousel: v3.3.0
  • react-native-gesture-handler: v2.9.0

Smartphone (please complete the following information):

  • Device: Android TV (Philips)
  • OS: Android 11
@Aure77 Aure77 added the bug Something isn't working label Feb 5, 2023
@gigby
Copy link

gigby commented Feb 9, 2023

Hi, bro @dohooo. You created nice library.
I have completely the same issue.
I use this lib to building calendars. I have two. First one I displaying data by month (just usual calendar) and it's rendering as expected according to windowSize prop (it has before 50 items (months) to render). Second one displaying data by weeks and it's rendering all the items each time (easy to test, just put console.log in renderItem method). Week view is much simpler than month but hundreds of rendering items make my test device hanging (even in release build).

Versions:

  • react: 17.0.2
  • react-native: 0.68.2
  • react-native-reanimated: 2.9.0
  • react-native-reanimated-carousel: 3.3.0
  • react-native-gesture-handler: 2.9.0

Smartphone

  • Apple iPhone Xs Max
  • OS: iOS 16.1.1
  • Any iOS Simulator
  • Android is not tested yet

PS: @Aure77 Did you find out the solution?

@Aure77
Copy link
Author

Aure77 commented Feb 9, 2023

I found that this render problem happen only on first render. After that, windowSize is taken in account.
So as workaround, I return "null" in renderItem only in first render to avoid a lot of JS work (ex: effect) on start.

@gigby
Copy link

gigby commented Feb 9, 2023

I found that this render problem happen only on first render. After that, windowSize is taken in account. So as workaround, I return "null" in renderItem only in first render to avoid a lot of JS work (ex: effect) on start.

Thank you. I already found the problem in my code. The renderItem method had external dependency which were causing huge number of re-renders.
It was issue on my code.
The library works nice

@tasgon
Copy link

tasgon commented Mar 24, 2023

If you wouldn't mind, could you elaborate on what you did to fix this? I'm currently running into the same issue.

EDIT: I was able to figure out why I had a large number of re-renders. It looks like I had the carousel's onSnapToItem set to a function that updated the state of the component holding the carousel, causing it to be re-rendered, which made the carousel itself re-render everything. I still have the problem of every view being rendered on the Carousel's first render, however.

@gigby
Copy link

gigby commented May 2, 2023

@tasgon I did small research as well and found out that this lib is not good for big data arrays. I started making the library without this limitation, but it's not ready for now.
This one is good for a small or middle data portion. But if you put into it an array with a length of 1000 or 10000 it will hang and glitch with any optimizations. This library is nice but not for this.

@klandell
Copy link

I created a PR to hopefully fix this, but it needs some more work with the unit testing. In the meantime you can use the following patch. I have tested it with all of the use cases I have, but more eyes would be helpful.

diff --git a/src/Carousel.tsx b/src/Carousel.tsx
index 980baac7e27bf8e28c1c21cd5217cf24e5b929ed..b4f19050e05fe34db6487b49a5e88b812feadfca 100644
--- a/src/Carousel.tsx
+++ b/src/Carousel.tsx
@@ -1,5 +1,4 @@
-/* eslint-disable @typescript-eslint/no-use-before-define */
-import React from "react";
+import React, { Fragment } from "react";
 import { StyleSheet } from "react-native";
 import { GestureHandlerRootView } from "react-native-gesture-handler";
 import { runOnJS, useDerivedValue } from "react-native-reanimated";
@@ -13,6 +12,7 @@ import { useOnProgressChange } from "./hooks/useOnProgressChange";
 import { usePropsErrorBoundary } from "./hooks/usePropsErrorBoundary";
 import { useVisibleRanges } from "./hooks/useVisibleRanges";
 import { BaseLayout } from "./layouts/BaseLayout";
+import { LazyView } from "./LazyView";
 import { ScrollViewGesture } from "./ScrollViewGesture";
 import { CTX } from "./store";
 import type { ICarouselInstance, TCarouselProps } from "./types";
@@ -171,9 +171,10 @@ const Carousel = React.forwardRef<ICarouselInstance, TCarouselProps<any>>(
           autoFillData,
         });
 
-        return (
+        const isLazy = !!windowSize;
+
+        const content = (
           <BaseLayout
-            key={i}
             index={i}
             handlerOffset={offsetX}
             visibleRanges={visibleRanges}
@@ -188,6 +189,8 @@ const Carousel = React.forwardRef<ICarouselInstance, TCarouselProps<any>>(
             }
           </BaseLayout>
         );
+
+        return isLazy ? <LazyView key={i} index={i} visibleRanges={visibleRanges}>{content}</LazyView> : <Fragment key={i}>{content}</Fragment>;
       },
       [
         loop,
@@ -198,6 +201,7 @@ const Carousel = React.forwardRef<ICarouselInstance, TCarouselProps<any>>(
         renderItem,
         layoutConfig,
         customAnimation,
+        windowSize,
       ],
     );
 
diff --git a/src/LazyView.tsx b/src/LazyView.tsx
index 5bc1cc0ea585992a68530dd5bc72ba3f6779a954..731cecbfd8f2ac5dfb77654275c7f40b479b3b9d 100644
--- a/src/LazyView.tsx
+++ b/src/LazyView.tsx
@@ -1,15 +1,32 @@
-import type { PropsWithChildren } from "react";
-import React from "react";
+import React, { type PropsWithChildren, useState } from "react";
+import { runOnJS, useAnimatedReaction } from "react-native-reanimated";
+
+import type { IVisibleRanges } from "./hooks/useVisibleRanges";
 
 interface Props {
-  shouldUpdate: boolean
+  index: number
+  visibleRanges: IVisibleRanges
 }
 
 export const LazyView: React.FC<PropsWithChildren<Props>> = (props) => {
-  const { shouldUpdate, children } = props;
+  const { index, visibleRanges, children } = props;
+
+  const [shouldRender, setShouldRender] = useState<boolean>(false);
+
+  useAnimatedReaction(
+    () => {
+      const { negativeRange, positiveRange } = visibleRanges.value;
+      return (index >= negativeRange[0] && index <= negativeRange[1]) || (index >= positiveRange[0] && index <= positiveRange[1]);
+    },
+    (currentValue, previousValue) => {
+      if (currentValue !== previousValue)
+        runOnJS(setShouldRender)(currentValue);
+    },
+    [index, visibleRanges],
+  );
 
-  if (!shouldUpdate)
-    return <></>;
+  if (!shouldRender)
+    return null;
 
   return <>{children}</>;
 };
diff --git a/src/layouts/BaseLayout.tsx b/src/layouts/BaseLayout.tsx
index 5447cef58ce10f09a2e11982ee0b8778159c4c2e..c827bb27733a3f259d670040737f9fc20a7d18f3 100644
--- a/src/layouts/BaseLayout.tsx
+++ b/src/layouts/BaseLayout.tsx
@@ -2,19 +2,15 @@ import React from "react";
 import type { ViewStyle } from "react-native";
 import type { AnimatedStyleProp } from "react-native-reanimated";
 import Animated, {
-  runOnJS,
-  useAnimatedReaction,
   useAnimatedStyle,
   useDerivedValue,
 } from "react-native-reanimated";
 
 import type { ILayoutConfig } from "./stack";
 
-import { useCheckMounted } from "../hooks/useCheckMounted";
 import type { IOpts } from "../hooks/useOffsetX";
 import { useOffsetX } from "../hooks/useOffsetX";
 import type { IVisibleRanges } from "../hooks/useVisibleRanges";
-import { LazyView } from "../LazyView";
 import { CTX } from "../store";
 
 export type TAnimationStyle = (value: number) => AnimatedStyleProp<ViewStyle>;
@@ -28,9 +24,8 @@ export const BaseLayout: React.FC<{
     animationValue: Animated.SharedValue<number>
   }) => React.ReactElement
 }> = (props) => {
-  const mounted = useCheckMounted();
   const { handlerOffset, index, children, visibleRanges, animationStyle }
-  = props;
+        = props;
 
   const context = React.useContext(CTX);
   const {
@@ -46,7 +41,6 @@ export const BaseLayout: React.FC<{
     },
   } = context;
   const size = vertical ? height : width;
-  const [shouldUpdate, setShouldUpdate] = React.useState(false);
   let offsetXConfig: IOpts = {
     handlerOffset,
     index,
@@ -77,28 +71,6 @@ export const BaseLayout: React.FC<{
     [animationStyle],
   );
 
-  const updateView = React.useCallback(
-    (negativeRange: number[], positiveRange: number[]) => {
-      mounted.current
-                && setShouldUpdate(
-                  (index >= negativeRange[0] && index <= negativeRange[1])
-                        || (index >= positiveRange[0] && index <= positiveRange[1]),
-                );
-    },
-    [index, mounted],
-  );
-
-  useAnimatedReaction(
-    () => visibleRanges.value,
-    () => {
-      runOnJS(updateView)(
-        visibleRanges.value.negativeRange,
-        visibleRanges.value.positiveRange,
-      );
-    },
-    [visibleRanges.value],
-  );
-
   return (
     <Animated.View
       style={[
@@ -109,16 +81,9 @@ export const BaseLayout: React.FC<{
         },
         animatedStyle,
       ]}
-      /**
-       * We use this testID to know when the carousel item is ready to be tested in test.
-       * e.g.
-       *  The testID of first item will be changed to __CAROUSEL_ITEM_0_READY__ from __CAROUSEL_ITEM_0_NOT_READY__ when the item is ready.
-       * */
-      testID={`__CAROUSEL_ITEM_${index}_${shouldUpdate ? "READY" : "NOT_READY"}__`}
+      testID={`__CAROUSEL_ITEM_${index}`}
     >
-      <LazyView shouldUpdate={shouldUpdate}>
-        {children({ animationValue })}
-      </LazyView>
+      {children({ animationValue })}
     </Animated.View>
   );
 };
diff --git a/src/layouts/ParallaxLayout.tsx b/src/layouts/ParallaxLayout.tsx
index dbc0c858bcf98552ebbe3ee805638e0aeebeafa8..e37984b4fd5f375ce91c18ea1236c22a4f4aaf64 100644
--- a/src/layouts/ParallaxLayout.tsx
+++ b/src/layouts/ParallaxLayout.tsx
@@ -3,8 +3,6 @@ import React from "react";
 import Animated, {
   Extrapolate,
   interpolate,
-  runOnJS,
-  useAnimatedReaction,
   useAnimatedStyle,
 } from "react-native-reanimated";
 
@@ -12,7 +10,6 @@ import type { ILayoutConfig } from "./parallax";
 
 import { useOffsetX } from "../hooks/useOffsetX";
 import type { IVisibleRanges } from "../hooks/useVisibleRanges";
-import { LazyView } from "../LazyView";
 import type { IComputedDirectionTypes } from "../types";
 
 export const ParallaxLayout: React.FC<PropsWithChildren<IComputedDirectionTypes<
@@ -39,8 +36,6 @@ export const ParallaxLayout: React.FC<PropsWithChildren<IComputedDirectionTypes<
     vertical,
   } = props;
 
-  const [shouldUpdate, setShouldUpdate] = React.useState(false);
-
   const size = props.vertical ? props.height : props.width;
 
   const x = useOffsetX(
@@ -103,27 +98,6 @@ export const ParallaxLayout: React.FC<PropsWithChildren<IComputedDirectionTypes<
     };
   }, [loop, vertical, parallaxScrollingOffset]);
 
-  const updateView = React.useCallback(
-    (negativeRange: number[], positiveRange: number[]) => {
-      setShouldUpdate(
-        (index >= negativeRange[0] && index <= negativeRange[1])
-                    || (index >= positiveRange[0] && index <= positiveRange[1]),
-      );
-    },
-    [index],
-  );
-
-  useAnimatedReaction(
-    () => visibleRanges.value,
-    () => {
-      runOnJS(updateView)(
-        visibleRanges.value.negativeRange,
-        visibleRanges.value.positiveRange,
-      );
-    },
-    [visibleRanges.value],
-  );
-
   return (
     <Animated.View
       style={[
@@ -135,7 +109,7 @@ export const ParallaxLayout: React.FC<PropsWithChildren<IComputedDirectionTypes<
         offsetXStyle,
       ]}
     >
-      <LazyView shouldUpdate={shouldUpdate}>{children}</LazyView>
+      {children}
     </Animated.View>
   );
 };

@smontlouis
Copy link

@klandell awesome work thank you very much !

dohooo added a commit that referenced this issue Dec 26, 2023
Before, even if the limit of the number of render is set, it will render one more layer of
BaseLayout, which makes the performance can not be maximized, and now the optimization makes
BaseLayout will not render any more, even if the number of data is 1 million, it will only render
the specified amount of render. Performance has improved dramatically.

fix #352, fix #362, fix #258, fix #478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants