Shopify / flash-list

A better list for React Native
https://shopify.github.io/flash-list/
MIT License
5.47k stars 281 forks source link

ItemSeperator not rendered correctly with useCallback #505

Open alexco2 opened 2 years ago

alexco2 commented 2 years ago

Current behavior

When loading in items in an infinitive scroll list (with onEndReached), the top most ItemSeperator of the item that is loaded in is not rendered. When scrolling away from the not rendered ItemSeperator and back again, the ItemSeperator is loaded in again correctly. The problem is with renderItem being memoized in a useCallback. Is that even necessary since the cells get reused anyway? If not, the fix is quite simple, but perhaps it should be mentioned in the docs.

Expected behavior

ItemSeperator should be rendered initially.

To Reproduce

const DATA = [
  {
    title: 'First Item',
  },
  {
    title: 'Second Item',
  },
  {
    title: 'First Item',
  },
  {
    title: 'Second Item',
  },
  {
    title: 'First Item',
  },
  {
    title: 'Second Item',
  },
  {
    title: 'First Item',
  },
  {
    title: 'Second Item',
  },
  {
    title: 'First Item',
  },
  {
    title: 'Second Item',
  },
];

const TestList: React.FC = () => {
  const [data, setdata] = useState(DATA);

  const onEndReached = () => {
    setdata(prev => [...prev, ...DATA]);
  };

  const renderItem = useCallback(() => {
    return <Text>test</Text>;
  }, []);
  return (
    <FlashList
      data={data}
      renderItem={renderItem}
      estimatedItemSize={20}
      ItemSeparatorComponent={() => (
        <View style={{height: 1, width: '100%', backgroundColor: 'red'}} />
      )}
      onEndReachedThreshold={0.1}
      onEndReached={onEndReached}
    />
  );
};

Platform:

Environment

1.0.4

naqvitalha commented 2 years ago

Good catch. I'll look into fixing this soon.

naqvitalha commented 2 years ago

You can actually skip memoizing renderItem with separators. The list's update logic doesn't depend on reference of this callback. I'll fix it anyway in the future.

alexco2 commented 2 years ago

My solution is to skip memorization as well as I already thought it is unnecessary anyway. I guess a short note in the docs would be enough. Good to know that the update logic is not affected though.

naqvitalha commented 2 years ago

@alexco2 Well there is an impact but it's limited to pagination. When you add more data to the list as of now it skips updating items that are already there. This is the reason why separator is not updating. I need to find a way to update the last item.

alexco2 commented 2 years ago

@naqvitalha thank you for the explanation :)

alexisloiselle commented 2 years ago

Memoizing the renderItem function also causes the items to not be re-rendered when a dependency changes.

teivienn commented 3 months ago

Are there any updates on this problem?

coyksdev commented 3 weeks ago

is there any update?