facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
118.1k stars 24.19k forks source link

VirtualizedList- inefficient function passing for CellRenderer #20174

Open jasekiw opened 6 years ago

jasekiw commented 6 years ago

Environment

Run react-native info in your terminal and paste its contents here.

Environment:
  OS: Windows 10
  Node: 8.11.1
  Yarn: 0.21.3
  npm: 5.8.0
  Watchman: Not Found
  Xcode: N/A
  Android Studio: Version  3.0.0.0 AI-171.4408382

Packages: (wanted => installed)
  react: 16.3.1 => 16.3.1
  react-native: ^0.55.2 => 0.55.4

Description

I used the npm package why-did-you-update to help optimize my components and prevent re-renders. However there is a re-render that I cannot control. This happens for every item in the list. My gues

CellRenderer.props: Changes are in functions only. Possibly avoidable re-render?
Functions before: {onLayout: ƒ}
Functions after: {onLayout: ƒ}

It seems this is caused by an anonymous function at https://github.com/facebook/react-native/blob/d756d94b3a7e2812f17f549c57767ac63734b49c/Libraries/Lists/VirtualizedList.js#L691

I propose that this function be defined on the class to prevent unnecessary re-renders.

patrickkempff commented 6 years ago

Thank you reporting this issue. It would be great if you could open a PR 👍

VahidBo commented 6 years ago

@jasekiw Do you want to open a PR for this issue?

jasekiw commented 6 years ago

@VahidBo Yes, I just committed some code. I will need to get the tests running before I feel comfortable making a pr.

jasekiw commented 6 years ago

@VahidBo How do I test my local react-native with a project? I tried npm link but when I run npm start which calls react-native-scripts start, it fails with the following error. The project was generated with create-react-native-app phone-app.

0:09:15: Starting packager...
***ERROR STARTING PACKAGER***
Starting React Native packager...
Scanning folders for symlinks in /Users/jasekiw/projects/phone-app/node_modules (24ms)

Warning! Your metro configuration contains a deprecated function "projectRoots". Please, consider changing it to "getProjectRoot" with "watchFolders" (if needed)

react-native info

  React Native Environment Info:
    System:
      OS: macOS High Sierra 10.13.2
      CPU: x64 Intel(R) Core(TM) i7-4578U CPU @ 3.00GHz
      Memory: 2.86 GB / 16.00 GB
      Shell: 3.2.57 - /bin/bash
    Binaries:
      Node: 8.10.0 - /usr/local/bin/node
      npm: 6.1.0 - /usr/local/bin/npm
    SDKs:
      iOS SDK:
        Platforms: iOS 11.2, macOS 10.13, tvOS 11.2, watchOS 4.2
      Android SDK:
        Build Tools: 25.0.3, 27.0.3
        API Levels: 23, 25, 26
    IDEs:
      Android Studio: 2.3 AI-162.3871768
      Xcode: 9.2/9C40b - /usr/bin/xcodebuild
    npmPackages:
      react: 16.3.1 => 16.3.1 
      react-native: ^0.55.2 => 1000.0.0 
    npmGlobalPackages:
      react-native-cli: 2.0.1
      react-native: 1000.0.0
VahidBo commented 6 years ago

@jasekiw Usually when I want to test my codes on a open-source project, first I init a project and then apply my change on the codes of open-source project that I want to contribute on it (actually I change the codes on node_modules folder) and then I test my changes. I can't say that it is the best solution to do what you want but it usually works for me.

jasekiw commented 6 years ago

@VahidBo I was able to get that to work. I found out that there is another issue that is causing the CellRenderer to re-render.

https://github.com/facebook/react-native/blob/6b1d99686d09ec6a73ecc440b785ee74cc972ac1/Libraries/Lists/VirtualizedList.js#L701

the parentProps is not equal by reference which causes a re-render. I'm not sure how I can get around this while also having it render when needed. Maybe do a shallow comparison of the props in a shouldComponentUpdate method? Any thoughts?

jasekiw commented 6 years ago

@VahidBo I'm thinking the solution is to pull in the shallowEqual algorithm and modify it to allow for exclusions. I would exclude the parentProps and then perform a shallow equal on the parentProps. What do you think of this approach?

jasekiw commented 6 years ago

@VahidBo I found out what's going on. When any component is passed to the parent, It always receives a new reference thus causing props to change. It looks like the props that have components being passed in are

ItemSeparatorComponent ListEmptyComponent ListFooterComponent ListHeaderComponent

As far as I can tell the CellRenderer only cares about ItemSeparatorComponent which is already passed as a prop.

This means I need to exclude all of these in the parent props to allow the CellRenderer to only render when needed.

Is this logic right? I think I am close to submitting a pr.

VahidBo commented 6 years ago

@jasekiw Sorry for late replay, I did not work on VirtualList component so I can't help you so much. I'm happy that you make your PR and I hope it is a great one.

sebas-bytelion commented 4 years ago

Any updates ?

jasekiw commented 4 years ago

@sebas-bytelion

There have been multiple PR's opened for this. The one that is currently opened is

https://github.com/facebook/react-native/pull/27115

I believe they are waiting for a review and merge from the React Native team.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.