saleel/react-native-super-grid

Optimize the components

IsaaX opened this issue · 4 comments

IsaaX commented

Hello! Loving the app as it does a great job overall!

So I've been using this component for some time now and while it does the job well, it does cause some needless rerenders that can be easily fixed if you were opened to making some modifications to the apps. I enabled this library https://github.com/welldone-software/why-did-you-render during my development to kink out performance issues early on and this is the last thing on the list.

Wanted to see if turning these components into PureComponents would intrigue you on the basis that SectionList https://reactnative.dev/docs/sectionlist and Flatlist https://reactnative.dev/docs/flatlist are PureComponents per docs so
More specifically using React.memo and turning these components into functional components to leverage hooks useCallback and useMemo and only updating when absolutely necessary.

I've already proved out the concept with SectionGrid and am happy to make a PR request for it. I have business specific specs locally that I leveraged in order to do this refactor and here are breaking changes that would occur. I would of added tests but theres no underlying structure for it so punted on that. But would suggest react-testing-library and using inlineSnapshots to do assertions regarding what gets rendered in the list.

IsaaX@f1c26b2

Breaking changes:
React - 16.8 would be required for hooks
References now get updated a bit relatively to what it was done previously
ref.current.sectionList.scrollToLocation -> ref.current.scrollToLocation due to forwardRef

Interesting read.

You are right. We should be able to improve the performance by changing it to React.memo pure component and using useCallback to do all the calculations. I would have probably done it this way if this library was created today.

I always wanted to add tests for all existing features/props, but never ended up doing it :-( If you can include tests, it would be great. react-testing-library is great choice 👍

We can do a major version bump when this is released. So minor breaking changes would not be a problem. I was also thinking of changing items to data in FlatGrid prop to make it in line with the FlatList.

I will be happy to see a PR on this. If you are only interested in doing this partially, I can do the rest. Let me know.

IsaaX commented

I've opened a PR for this then #131

For now, I'm going to opt out of doing the specs but I'll be utilizing this library fairly close that I'll more than likely circle around when I need more modifications to the library (although more than likely not :) ) Also I don't have a use case for FlatGrid so I'll punt off those changes as well.

Thanks for the quick reply! Great job on the app, it's been easy to work with.

edit: let me know if you want any changes. I tried to do the least amount of changes.

Thank you.

I have merged your PR to a new branch v4.

I will update the FlatGrid when I have some time. I will also need to update the versions, readme and migration details...etc.

Will merge v4 to master branch when its good to go.

IsaaX commented

Perfect! I just found an issue migrating over to this new component

for some odd reason onLayout is not being invoked, I tried the old version compared to the new version and onLayout gets invoked per usual. Not sure why this is the case and I've looked around and have not seen anything concrete.

FWIW it's on an android device.

      <View onLayout={onLocalLayout}>
      <SectionList
        sections={groupedSections}
        keyExtractor={(rowItems, index) => {
          if (keyExtractor) {
            return rowItems
              .map((rowItem, rowItemIndex) => {
                return keyExtractor(rowItem, rowItemIndex);
              })
              .join('_');
          } else {
            return `row_${index}`;
          }
        }}
        style={style}
        ref={ref}
        {...restProps}
      />
      </View>

wrapping it around a View seems to fix it. So i'm going to keep iterating on it and try to kink out any issues for you.

Ill be submitting another PR here within the next day once I kink out any issues with the memoizing and what not.