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 + getItemLayout only renders initialNumToRender items #15734

Closed
PublicParadise opened this issue Sep 1, 2017 · 17 comments
Closed
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@PublicParadise
Copy link

PublicParadise commented Sep 1, 2017

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

  1. react-native -v: 0.47.2
  2. node -v: v6.10.3
  3. npm -v: 3.10.10
  4. yarn --version: n/a

Then, specify:

  • Target Platform: iOS
  • Development Operating System: macOS
  • Build tools: Xcode

Steps to Reproduce

(Write your steps here:)

  1. Create a component with a FlatList w/t getItemLayout, i.e.
<FlatList
            ref={ref => { const anyRef: any = ref; this._flatList = anyRef }}
            data={data}
            renderItem={this._renderItem}
            onViewableItemsChanged={this._onViewableItemsChanged}
            getItemLayout={this._getItemLayout}
            initialNumToRender={10}
          />

I am using the standard getItemLayout:

getItemLayout={(data, index) => (
  {length: ITEM_HEIGHT, offset: ITEM_HEIGHT * index, index}
)}
  1. Provide a data source that has more than initialNumToRender items
  2. Render and scroll to bottom

Expected Behavior

(Write what you thought would happen.)

All items are rendered.

Actual Behavior

Only initialNumToRender items are rendered.

(Write what happened. Add screenshots!)

Reproducible Demo

The bug is in VirtualizedList:

const onLayout =
      getItemLayout && !parentProps.debug && !fillRateHelper.enabled()
        ? undefined
        : this.props.onLayout;

The fix is:

const onLayout = this.props.onLayout;

You need onLayout because somebody needs to call _onCellLayout(e, key, ii).

(Paste the link to an example project and exact instructions to reproduce the issue.)

@duailibe
Copy link

duailibe commented Sep 6, 2017

Hey, I've just run to a similar problem: the FlatList w/ getItemLayout would render the initialNumToRender and then have a blank space below related to the additional items not being rendered. And when I removed getItemLayout it would render the additional items but only after scrolling. But the most interesting part is that that would not happen all the time, only when the app would go through a specific flow.

After some debugging with the RN internals, I've discovered that we had a never ending animation that was preventing FlatList to update, since it uses InteractionManager.runAfterInteractions(). When I fixed that, everything started working again.

Also, with your fix, getItemLayout is ignored and I don't want that in my use case.

Hope it helps!

@PublicParadise
Copy link
Author

@duailibe Interesting... In my case my fix does not lead to situations where getItemLayout gets ignored. I found this workaround after I added debug={true}. My code also doesn't have any infinite animations and does not use InteractionManager.runAfterInteractions(). Besides, why should that matter? Just by looking at the code it's clear to me that the getItemLayout path has not been fully explored. I did notice that other frameworks like react-native-snap-carousel somehow worked around all the quirks and got something up and running with getItemLayout. Their code is full of comments that point to tons of problems of the current FlatList implementation.

I am also not too happy about the turnaround times for bug triages. I'll close this bug in 3 weeks if nobody from Facebook has looked at it by then (as I have done before).

@PublicParadise
Copy link
Author

After working with getItemLayout plus my proposed fix for a few weeks I have come to the conclusion that getItemLayout cannot be used in production code.

Here are my recommendations:

  1. Avoid getItemLayout. It's not working correctly. I would classify the quality of the code as experimental at best.
  2. Avoid scrollToIndex, scrollToItem, and scrollToEnd , because they require getItemLayout. Instead use scrollToOffset, which comes with fewer bugs.
  3. Consider changing the documentation and let clients know about the experimental status of getItemLayout. Something along the lines of the removeClippedSubviews warning:

Note: may have bugs (missing content) in some circumstances - use at your own risk.

  1. Please do something about the bug triage turn around times.

Good luck, I am closing this issue.

@dotansimha
Copy link

@PublicParadise
I think that the only possible fix for this issue it not to use getItemLayout, but then the result is that the scroller is rendered only for the initial rendered count, and you can't use scrollTo... functions.
When using debug=true, the onLayout is not undefined and everything works.

Your idea of changing onLayout logic is perfect, and I think you need to reopen this issue until this issue is resolved.

@dotansimha
Copy link

My current workaround is to override FlatList and VirtualizedList and override the behaviour or getLayout calculation:

// custom-flat-list.js

import React from 'react';
import { FlatList } from 'react-native';
import { CustomVirtualizedList } from "./custom-virtualized-list";

export class CustomFlatList extends FlatList {
  render() {
    return (
      <CustomVirtualizedList
        {...this.props}
        renderItem={this._renderItem}
        getItem={this._getItem}
        getItemCount={this._getItemCount}
        keyExtractor={this._keyExtractor}
        ref={this._captureRef}
        viewabilityConfigCallbackPairs={this._virtualizedListPairs}
      />
    )
  }
}

// custom-virtualized-list.js

import React from 'react';
import { VirtualizedList } from 'react-native';

export class CustomVirtualizedList extends VirtualizedList {
  constructor(props, context) {
    super(props, context);
    this._fillRateHelper._enabled = true;
  }
}

This way, getLayout always has a valid value, and the items layout is calculated and the items appear.

@PublicParadise

@immoskito
Copy link

@dotansimha Your workaround is much cleaner than what I am doing, which is patching the code in my postInstall.sh. My package.json automatically runs a shell script called postInstall.sh:

  "scripts": {
    "postinstall": "./scripts/postInstall.sh",
    ...

My postInstall.sh then crudely patches VirtualizedList.js:

# VirtualizedList + getItemLayout only renders initialNumToRender items
# @see https://github.com/facebook/react-native/issues/15734
# getItemLayout does not work
sed -i.bak -e 's/const onLayout =$/const onLayout = false \&\&/' node_modules/react-native/Libraries/Lists/VirtualizedList.js

I was happy for about two weeks. Then I realized that my fix only worked for small lists. Once I used lists with about 1100 items everything fell apart. For example I did things like: scroll to item # 678 followed by manually scrolling one or two pages up. The result was often if not always a blank screen.

I switched back to removing my patch and avoiding getItemLayout. But I still wanted to scroll to items. I found that scrollToOffset works better. Don't get me wrong, there are still plenty of bugs left. But at least I don't see blank screens that often. It's amazing that even after about two years of development FlatList and VirtualizedList haven't been able to grow out of their "hobbyist" quality stage. What a bummer. At our company we would probably have hit the reset button on this component much earlier: Let somebody else try a new implementation, the current approach is probably not going anywhere.

@PublicParadise
Copy link
Author

Just stumbled over this issue that touches on some of the current problems with FlatList: meliorence/react-native-snap-carousel#201

In particular:

There are flaws with React Native's implementation, as you can see by looking at the dedicated issues. I recommend that you take a look at these and see if users have come up with workarounds.

as well as this note for Android:

This is unfortunate, but it's rooted in various flaws of ScrollView/FlatList's implementation and the miscellaneous workarounds we had to implement to compensate for it.

I find the list of dedicated FlatList issues quite discouraging...

@PublicParadise
Copy link
Author

PublicParadise commented Nov 1, 2017

FYI:

Vote for React Native's feature requests so we can improve the plugin
meliorence/react-native-snap-carousel#203

bd-arc pushed a commit to meliorence/react-native-snap-carousel that referenced this issue Nov 1, 2017
…s but the initial ones to be rendered

Fixes #193.
Modifications are based on what is discussed in [this
thread](facebook/react-native#15734).
@Elijen
Copy link

Elijen commented Jan 11, 2018

Why is this issue closed? If getItemLayout is not working correctly the issue should stay open.

@tarikmlaco
Copy link

This issue hasn't been solved yet. Please reopen. Also...I noticed getItemLayout makes problems only if setState is triggered inside the component...has anyone else noticed this?

@lishoulong
Copy link

when use getItemLayout, After pull request the api, the page will first white for a moment, and then the content show. This video show the phenomenon, https://youtu.be/SmsEalsACQQ

@DylanVann
Copy link
Contributor

Issue should still be open.

@deecewan
Copy link
Contributor

Bump this. Spent ages trying to figure out why I had no items rendering outside of the initial ones that rendered, only to stumble onto this issue some time later. Clarify that it works fine on iOS, and when debugging remotely on Android. It's only the production version of Android that it fails (it works if the debug flag is set).

This can't be intended behaviour. Is no-one at FB using FlatList?

@PublicParadise
Copy link
Author

@deecewan The problem is that FlatList works in "most of the cases". But unfortunately not for me. It sounds like it doesn't work for you either. Here is my wild guess: It has to do with animations. Above you'll find notes that point to InteractionManager.runAfterInteractions(). The suggested solutions didn't apply to my code. But I think there is some weird dependency to InteractionManager.runAfterInteractions() that bites some of us in the butt. My project uses fairly complex animations and the item heights can be different. When I was debugging the problems back in Aug 2017 I got the impression that the code was just plain wrong - the code didn't seem coherent. That might have changed.

What I saw cannot be unseen, though. For that reason I am supporting @bd-arc if he wants to replace FlatList, see meliorence/react-native-snap-carousel#250 .

@deecewan
Copy link
Contributor

oh, no. mine is quite simple - horizontal scrolling a full-width 'page', so the width never changes. there is also no animation.

making the change you suggested at the top (const onLayout = this.props.onLayout) seems to work in my case, so I'm going to stick with it. I just am not really sure of what it's doing, to be honest, but it works and I can't see a good reason not to - it's the default if getItemLayout isn't provided, anyways.

I haven't looked at it (or anything similar), but I'm not sure why it's so difficult to build? Render empty placeholders everywhere if the index is outside of currentIndex ± 5, otherwise call the renderItem callback. I assume there is some more complication to it than that, but there does seem to be a lot of complexity.

@esprehn
Copy link
Contributor

esprehn commented Jun 21, 2018

This bug should definitely still be open. If the getItemLayout prop is passed then SectionList, FlatList and VirtualizedList all seem to be broken where you have a large blank space inside the list, but if you don't pass the prop then you can't use any of the scroll methods.

@gaearon You wanted people to ping you when there's critical issues that aren't being addressed. This is a good example.

@goncalvesej
Copy link

My workaround:

initialNumToRender={data.length}

so it renders the whole list.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Sep 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests