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

VirtualizedList- inefficient function passing for CellRenderer #20174

Open
2 of 3 tasks
jasekiw opened this issue Jul 12, 2018 · 12 comments
Open
2 of 3 tasks

VirtualizedList- inefficient function passing for CellRenderer #20174

jasekiw opened this issue Jul 12, 2018 · 12 comments
Labels
Bug Component: VirtualizedList Help Wanted :octocat: Issues ideal for external contributors. JavaScript Newer Patch Available Resolution: PR Submitted A pull request with a fix has been provided.

Comments

@jasekiw
Copy link

jasekiw commented Jul 12, 2018

  • Review the documentation
  • Search for existing issues
  • Use the latest React Native release - using version 0.55.2 did not see any changes to the FlatListComponent since that version.

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

onLayout={e => this._onCellLayout(e, key, ii)}

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

@jasekiw jasekiw changed the title FlatList component inefficient function passing for CellRenderer FlatList - inefficient function passing for CellRenderer Jul 12, 2018
@patrickkempff
Copy link
Contributor

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

@patrickkempff patrickkempff added Help Wanted :octocat: Issues ideal for external contributors. JavaScript 🔶Lists labels Jul 12, 2018
@VahidBo
Copy link
Contributor

VahidBo commented Jul 12, 2018

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

@jasekiw
Copy link
Author

jasekiw commented Jul 12, 2018

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

@jasekiw
Copy link
Author

jasekiw commented Jul 13, 2018

@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

@jasekiw jasekiw changed the title FlatList - inefficient function passing for CellRenderer VirtualizedList- inefficient function passing for CellRenderer Jul 13, 2018
@VahidBo
Copy link
Contributor

VahidBo commented Jul 13, 2018

@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
Copy link
Author

jasekiw commented Jul 14, 2018

@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.

parentProps={this.props}

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
Copy link
Author

jasekiw commented Jul 14, 2018

@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
Copy link
Author

jasekiw commented Jul 14, 2018

@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
Copy link
Contributor

VahidBo commented Jul 16, 2018

@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
Copy link

Any updates ?

@jasekiw
Copy link
Author

jasekiw commented Mar 19, 2020

@sebas-bytelion

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

#27115

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

lunaleaps pushed a commit to lunaleaps/react-native that referenced this issue Aug 4, 2021
Summary:
This is VirtualList Optimization done by christianbach in facebook#21163 enabled behind a prop: `experimentalVirtualizedListOpt` with backward compatibility.

Fixes facebook#20174 based of jasekiw pull request facebook#20208

## Changelog

// TODO:

[CATEGORY] [TYPE] - Message

Pull Request resolved: facebook#27115

Test Plan:
// TODO:
Add tests related to backward compatibility. (Will need help)

Differential Revision: D30095387

Pulled By: lunaleaps

fbshipit-source-id: 1c41e9e52beeb79b56b19dfb12d896a2c4c49529
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

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.

@github-actions github-actions bot added Stale There has been a lack of activity on this issue and it may be closed soon. and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Component: VirtualizedList Help Wanted :octocat: Issues ideal for external contributors. JavaScript Newer Patch Available Resolution: PR Submitted A pull request with a fix has been provided.
Projects
None yet
6 participants