Disabling pull-to-refresh on FlatList causes all items to be re-created

[maxschmeling]

🐛 Bug Report

When disabling pull-to-refresh in a FlatList by setting onRefresh to undefined every item in the list gets unmounted and recreated.

I looked into why this would be happening and it appears to be because of this code in ScrollView:

react-native/Libraries/Components/ScrollView/ScrollView.js

Lines 1082 to 1120 in f741d33

	const refreshControl = this.props.refreshControl;
	if (refreshControl) {
	if (Platform.OS === 'ios') {
	// On iOS the RefreshControl is a child of the ScrollView.
	// tvOS lacks native support for RefreshControl, so don't include it in that case
	return (
	// $FlowFixMe
	<ScrollViewClass {...props} ref={this._setScrollViewRef}>
	{Platform.isTV ? null : refreshControl}
	{contentContainer}
	</ScrollViewClass>
	);
	} else if (Platform.OS === 'android') {
	// On Android wrap the ScrollView with a AndroidSwipeRefreshLayout.
	// Since the ScrollView is wrapped add the style props to the
	// AndroidSwipeRefreshLayout and use flex: 1 for the ScrollView.
	// Note: we should only apply props.style on the wrapper
	// however, the ScrollView still needs the baseStyle to be scrollable
	return React.cloneElement(
	refreshControl,
	{style: props.style},
	<ScrollViewClass
	{...props}
	style={baseStyle}
	// $FlowFixMe
	ref={this._setScrollViewRef}>
	{contentContainer}
	</ScrollViewClass>,
	);
	}
	}
	return (
	// $FlowFixMe
	<ScrollViewClass {...props} ref={this._setScrollViewRef}>
	{contentContainer}
	</ScrollViewClass>
	);

To Reproduce

1. Add a FlatList with some data and onRefresh and refreshing set to valid values
2. Change onRefresh and refreshing to undefined

Expected Behavior

The item components already rendered should be reused

Code Example

https://snack.expo.io/@maxschmeling/disrespectful-toffee

Environment

  React Native Environment Info:
    System:
      OS: macOS 10.14.3
      CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
      Memory: 8.02 MB / 16.00 GB
      Shell: 5.3 - /bin/zsh
    Binaries:
      Node: 11.3.0 - ~/.nvm/versions/node/v11.3.0/bin/node
      Yarn: 1.13.0 - /usr/local/bin/yarn
      npm: 6.9.0 - ~/.nvm/versions/node/v11.3.0/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
    IDEs:
      Android Studio: 3.1 AI-173.4907809
      Xcode: 10.1/10B61 - /usr/bin/xcodebuild
    npmPackages:
      react: ^16.8.3 => 16.8.4 
      react-native: ^0.59.0 => 0.59.0 
    npmGlobalPackages:
      react-native-git-upgrade: 0.2.7

[thymikee]

Hey, we've confirmed this is an issue. I think I have an idea on how to fix it for iOS (we need to move the platform check to the inside of ScrollViewClass so we don't re-mount it), but it may get trickier on Android (need to investigate).

I'm gonna work on this later today, but if anyone wants to take a stab at it, please do so :)

[michalchudziak]

While fixing this on iOS seems like a straight-forward job, avoiding unmounting on Android might not be doable without the support of re-parenting in React facebook/react#3965.

SwipeRefreshLayout requires us to wrap the ScrollView with it, and from the React perspective, it will always invalidate the tree.

Perhaps we could always wrap ScrollView with SwipeRefreshLayout, and conditionally disable it, but it doesn't seem like a good idea.

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.