Fix sporadic issue with onEndReached called on load when not needed

Summary:
Fixes #16067

The issue is due to a race between `onLayout` and `onContentSizeChange`, which in general should be fine because there is no expectation of ordering between the two, and only causes issues with certain configurations.

The bug can be triggered if `initialNumToRender` is smaller than needed to fill past the `onEndReachedThreshold` (say the default, 10, is only 580px tall, but it takes 15 to reach the threshold). This will cause an incrementally render of more items to try and fill the viewport. The problem is that if the `onLayout` comes back before the first `onContentSizeChange`, it will first do the state increment to render 20 items and then the stale `onContentSizeChange` callback from 10 items will fire and we'll think that the content size for 20 items is 580px when in fact it's 1160px (which is past the threshold). If those 20 items are also all of our available data, then we'll call `onEndReached` because we think we've rendered everything and are still within the `onEndReachedThreshold`.

The fundamental problem here is the system getting confused when a stale async `onContentSizeChange` comes in after increasing `state.last`. I wish there was a concrete timeframe, but Fabric will give us more flexibility to do things synchronously so hopefully we can avoid class of issues once that roles out.

The fix here simply adds a check to make sure `contentLength` has been set before adjusting the render window so it's not possible to increase the window size before the initial `onContentSizeChange` callback fires.

For completeness, there are a few user-code workarounds to avoid this issue entirely:

1) Provide the `getItemLayout` prop so the list doesn't have to rely on async layout data (you should do this whenever possible anyway for better perf). e.g. for the original snack example, you can just add `getItemLayout={(d, index) => ({length: 58, offset: 58 * index, index})}` since all the rows are height 58 and the issue will no longer repro. Note this is fragile and must be kept in sync with UI changes, a11y font scaling, etc - a more robust approach could be to render a single representative row offscreen and measure it with `onLayout` then use that value.
2) If `getItemLayout` is not feasible to compute for your UI, increase `initialNumToRender` to cover the `onEndReachedThreshold`.
3) And/or add your own logic to protect against extra calls to `onEndReached` as others have suggested.

Changelog:
[General][Fixed] - Fix sporadic issue with onEndReached called on load when not needed

# Test Plan
Adds a new jest test that fails without this fix and succeeds with it.

Reviewed By: TheSavior

Differential Revision: D18966721

fbshipit-source-id: de05d9f072e24a2faf351e7f5d60578a31def996

Libraries/Lists/VirtualizedList.js

@@ -1708,12 +1708,13 @@ class VirtualizedList extends React.PureComponent<Props, State> {
     }
     this.setState(state => {
       let newState;
+      const {contentLength, offset, visibleLength} = this._scrollMetrics;
       if (!isVirtualizationDisabled) {
         // If we run this with bogus data, we'll force-render window {first: 0, last: 0},
         // and wipe out the initialNumToRender rendered elements.
         // So let's wait until the scroll view metrics have been set up. And until then,
         // we will trust the initialNumToRender suggestion
-        if (this._scrollMetrics.visibleLength) {
+        if (visibleLength > 0 && contentLength > 0) {
           // If we have a non-zero initialScrollIndex and run this before we've scrolled,
           // we'll wipe out the initialNumToRender rendered elements starting at initialScrollIndex.
           // So let's wait until we've scrolled the view to the right place. And until then,
@@ -1728,7 +1729,6 @@ class VirtualizedList extends React.PureComponent<Props, State> {
           }
         }
       } else {
-        const {contentLength, offset, visibleLength} = this._scrollMetrics;
         const distanceFromEnd = contentLength - visibleLength - offset;
         const renderAhead =
           /* $FlowFixMe(>=0.63.0 site=react_native_fb) This comment suppresses
@@ -1772,6 +1772,13 @@ class VirtualizedList extends React.PureComponent<Props, State> {
           }
         }
       }
+      if (
+        newState != null &&
+        newState.first === state.first &&
+        newState.last === state.last
+      ) {
+        newState = null;
+      }
       return newState;
     });
   };

Libraries/Lists/__tests__/VirtualizedList-test.js

@@ -69,11 +69,12 @@ describe('VirtualizedList', () => {
 
   it('throws if no renderItem or ListItemComponent', () => {
     // Silence the React error boundary warning; we expect an uncaught error.
+    const consoleError = console.error;
     jest.spyOn(console, 'error').mockImplementation(message => {
       if (message.startsWith('The above error occured in the ')) {
         return;
       }
-      console.errorDebug(message);
+      consoleError(message);
     });
 
     const componentFactory = () =>
@@ -334,4 +335,53 @@ describe('VirtualizedList', () => {
     expect(scrollRef.measureLayout).toBeInstanceOf(jest.fn().constructor);
     expect(scrollRef.measureInWindow).toBeInstanceOf(jest.fn().constructor);
   });
+  it("does not call onEndReached when it shouldn't", () => {
+    const ITEM_HEIGHT = 40;
+    const layout = {width: 300, height: 600};
+    let data = Array(20)
+      .fill()
+      .map((_, key) => ({key: String(key)}));
+    const onEndReached = jest.fn();
+    const props = {
+      data,
+      initialNumToRender: 10,
+      renderItem: ({item}) => <item value={item.key} />,
+      getItem: (items, index) => items[index],
+      getItemCount: items => items.length,
+      getItemLayout: (items, index) => ({
+        length: ITEM_HEIGHT,
+        offset: ITEM_HEIGHT * index,
+        index,
+      }),
+      onEndReached,
+    };
+
+    const component = ReactTestRenderer.create(<VirtualizedList {...props} />);
+
+    const instance = component.getInstance();
+
+    instance._onLayout({nativeEvent: {layout}});
+
+    // We want to test the unusual case of onContentSizeChange firing after
+    // onLayout, which can cause https://github.com/facebook/react-native/issues/16067
+    instance._onContentSizeChange(300, props.initialNumToRender * ITEM_HEIGHT);
+    instance._onContentSizeChange(300, data.length * ITEM_HEIGHT);
+    jest.runAllTimers();
+
+    expect(onEndReached).not.toHaveBeenCalled();
+
+    instance._onScroll({
+      timeStamp: 1000,
+      nativeEvent: {
+        contentOffset: {y: 700, x: 0},
+        layoutMeasurement: layout,
+        contentSize: {...layout, height: data.length * ITEM_HEIGHT},
+        zoomScale: 1,
+        contentInset: {right: 0, top: 0, left: 0, bottom: 0},
+      },
+    });
+    jest.runAllTimers();
+
+    expect(onEndReached).toHaveBeenCalled();
+  });
 });