alexZajac / react-native-skeleton-content-nonexpo

A customizable skeleton-like loading placeholder for react native projects not using expo.
MIT License
308 stars 62 forks source link

WIP perf: add `PureSkeletonContent` component to improve performance by reducing re-rendering of component #29

Open mernxl opened 4 years ago

mernxl commented 4 years ago

This is a proof of concept as well as a final code to support this issue #28 i opened earlier.

As a guide, this section has been added to the README file.

PureSkeletonContent

If you are really concerned about performance, or you don't want your components mounting being runned while isLoading is false, then you may need to consider using PureSkeletonContent.

Point to note

All props passed to PureSkeletonContent should be a constant prop, memoize it if it will change sometime. Otherwise, you should consider using the good old SkeletonContent.

This point does not apply to componentProps as it is shallow checked to know if we should re-render the Skeleton or not.

import { FunctionComponent } from 'react';
import { Text, TextStyle, StyleSheet } from 'react-native';
import { PureSkeletonContent } from 'react-native-skeleton-content-nonexpo'; 

const Greetings: FunctionComponent<{ name: string }> = ({ name }) => 
  (<Text>Hello {name}</Text>);

const GreetingsSC = [{ height: 40, width: 200, paddingVertical: 2 }];

const SomeComponent: FunctionComponent<{name: string}> = ({ name }) => {
  const [loading, setLoading] = useState(true);

  return (
    <PureSkeletonContent 
      isLoading={isLoading}
      layout={GreetingsSC}
      component={Greetings} 
      componentProps={{ name }} // will be shallow checked, you don't need to memoize this
      containerStyle={styles.container} // notice we using styles from styleSheet
    />
  )
}

const styles = StyleSheet({
  container: {
    flex: 1, width: 300
  }
})
reichhartd commented 2 years ago

@alexZajac @mernxl Is there an update here? Would be very interested in this feature. Thanks for your effort!

alexZajac commented 2 years ago

Thanks for the PR! We would need the checks to pass for it to be merged!

mernxl commented 2 years ago

Hi, this MR will need some more review and probably some talks. If possible @alexZajac could you test on a local project? It's been a while I worked on such a project.

I am available to help drive this towards a positive direction.

My initial change was we use Animated from react-native and ditch reanimated. But then it's been a while, it's possible there's been some updates that fixes a lot. Let's just take the time to review and see how to get to a good point.

alexZajac commented 2 years ago

Hi!

Yeah, this one slipped over my hands with the amount of work I had on the side, not sure if having a different component is the way to go here. My rationale is that if this one is "more performant" than the other SkeletonContent, then why don't we replace the implementation of this one instead? Maybe there is a functional difference I didn't catch there.

Thoughts?

mernxl commented 2 years ago

Actually replacing without deprecating the current will make the community unstable. It will take time to migrate, most people will move away from the library. I would say I actually use them both within my project, for huge components, I will go with Pure, when I have small things to display within, I go with the current.

The primary function of the PureSkeletonContent is to allow you to pass in the "child component", it will only be initialized when isLoading is false and destroyed when true. I think a performance gain for navigating to new screens actually, where it does not create a component when it's not needed yet.

The current SkeletonContent, takes in an already initialized "child component", and so sometimes you need to check isLoading here and there especially when the data to be displayed drives the isLoading variable.