brh55 / react-native-masonry

:raised_hands: A pure JS react-native component to render a masonry~ish layout for images with support for dynamic columns, progressive image loading, device rotation, on-press handlers, and headers/captions.
MIT License
1.32k stars 157 forks source link

[VirtualizedList] - Warning: Each child in an array or iterator should have a unique "key" prop. #51

Closed soukupl closed 7 years ago

soukupl commented 7 years ago

Hello, there is a warning in render method of VirtualizedList. Not sure if this is your problem... I wasn't able to find it in code yet. It may be a problem of one of the dependencies... Can you please look at it? It's only a warning, but still... :)

` Warning: Each child in an array or iterator should have a unique "key" prop.

Check the render method of VirtualizedList. See https://fb.me/react-warning-keys for more information. in CellRenderer (at VirtualizedList.js:463) in VirtualizedList (at FlatList.js:557) in FlatList (at Column.js:129) in RCTView (at View.js:113) in View (at Column.js:121) in Column (at Masonry.js:118) in StaticRenderer (at ListView.js:464) in RCTScrollContentView (at ScrollView.js:707) in RCTScrollView (at ScrollView.js:800) in ScrollView (at ListView.js:332) in ListView (at Masonry.js:113) in RCTView (at View.js:113) in View (at Masonry.js:112) in Masonry (at PageGallery.js:13) in PageGallery (at App.js:203) in RCTScrollContentView (at ScrollView.js:707) in RCTScrollView (at ScrollView.js:800) in ScrollView (at KeyboardAwareScrollView.js:27) in KeyboardAwareScrollView (at Content.js:10) in Content (at connectStyle.js:384) in Styled(Content) (at App.js:302) in RCTView (at View.js:113) in View (at Container.js:19) in Container (at connectStyle.js:384) in Styled(Container) (at App.js:252) in RCTView (at View.js:113) in View (at index.js:570) in RCTView (at View.js:113) in View (at index.js:557) in Drawer (at index.js:10) in Drawer (at App.js:234) in RCTView (at View.js:113) in View (at Root.js:14) in Root (at connectStyle.js:384) in Styled(Root) (at App.js:233) in App (at registerRootComponent.js:36) in RootErrorBoundary (at registerRootComponent.js:35) in ExpoRootComponent (at renderApplication.js:35) in RCTView (at View.js:113) in View (at AppContainer.js:100) in RCTView (at View.js:113) in View (at AppContainer.js:121) in AppContainer (at renderApplication.js:34) `

brh55 commented 7 years ago

@soukupl Shouldn't be too hard to fix (probably need a key within either the columns or brick). I can see if I can apply a fix to this by next week, but will leave open for anyone to grab and try 👍

soukupl commented 7 years ago

@brh55 Well... I was able to track it down to _keyExtractor function in Column.js on line 117.

Your code is _keyExtractor = item => item.id || item.key;

It stops raising warnings after changing it to _keyExtractor = item => { return "image-key-" + (item.index || item.key); };

for some reason, _keyExtractor = item => item.index || item.key; is not working?

The main issue: there seems not to be the item.id, but there is an item.index... item.index seems to be the right variable to pass as the key.

The code _keyExtractor = item => item.index; also works without warnings... But I'm not sure why you have the item.id || item.key... So maybe if you look at it... You can see the whole picture of your code... I may still be missing something... But it seems to me, that the _keyExtractor should be only this _keyExtractor = item => item.index;

brh55 commented 7 years ago

_keyExtractor = item => item.index || item.key probably isn't working because the bricks don't have id or a key causing a bunch of "undefined" keys.

I would probably suggest id first, key second, and uri as the last resort, something like: _keyExtractor = item => (item.index || item.key || item.uri)? -- this is assuming that you wouldn't want duplicated images.

The problem with index is if you add an item anywhere other than the end of the list, the indexes will change and force a refresh on the entire column :(. What are your thoughts?

soukupl commented 7 years ago

Yes, the best solution might be some kind of image id (specified in the config array with image, it can be called simply id) and if not present maybe hashed uri (as uri probably can't be a key, or can it). The (optional) image id would solve the problem if you want to have duplicate image uris.

Can try to make something tomorrow and I will post here my new _keyExtractor (or I can make pull request).

brh55 commented 7 years ago

Indeed, well we can also state that a user must add an id or key property if they plan on rendering duplicated images. Hence, it would go for id or key first, then fall back to uri as a default (any string can be a key 😸 ).

srameshr commented 7 years ago

@brh55 I do add unique key and also a unique id for every record. But it still shows up. Especially on pull to refresh.

Here is what happens.

  1. I render 10 images on component mount
  2. On pull to refresh I call the api again and when it comes back the url's of some of the images have been changed. But the keys of that object remains the same.
    // On component mount
    {
    key: '1',
    id: '1',
    url: 'http://name.com/1.jpg',
    data: {}
    },
    {
    key: '2',
    id: '2',
    url: 'http://name.com/2.jpg',
    data: {}
    }

Now after I refresh via pull to refresh, I get the below data back

  // On pull to refresh
  {
    key: '1',
    id: '1',
    uri: 'http://name.com/hello.jpg',
    data: {}
  },
{
    key: '2',
    id: '2',
    uri: 'http://name.com/world.jpg',
    data: {}
  }

As you can see, the uri url has been changed, but the keys remain the same. At this point I see duplicate images in the masonry and the errors:

FlattenChildren encountered two children with the same key "1", in FlatList Column.js 129
FlattenChildren encountered two children with the same key "2", in FlatList Column.js 129
soukupl commented 7 years ago

ok, so I now have this:

_keyExtractor = item => "IMAGE-KEY-" + item.uri + "---" + (item.data.key ? item.data.key : "0");

URI of the image is used as primary key, with optional "key" from config...

<Masonry bricks=[ { uri: 'http://image1.jpg', key: 'image01' }, { uri: 'http://image2.jpg', key: 'image02' }, { uri: 'http://image3.jpg' } ] />

resulting to keys like (long image URI - shortened in this example): with key specified: IMAGE-KEY-https://lh3.googleusercontent.com/6Uzp...TZJU=s1600---image5 without key specified: IMAGE-KEY-https://lh3.googleusercontent.com/Ie...PSjy=s1600---0

srameshr commented 7 years ago

@soukupl Any idea why it happens on refresh.

brh55 commented 7 years ago

@soukupl I think that solution works well, it's rather consistent behavior. I don't see why you should be having any issues with refresh now with: _keyExtractor = item => "IMAGE-KEY-" + item.uri + "---" + (item.data.key ? item.data.key : "0")

soukupl commented 7 years ago

@brh55 seem to work for me too. Should I make a pull request or will you push the change with your next update?

brh55 commented 7 years ago

@soukupl Feel free to PR :), also need to do the following:

soukupl commented 7 years ago

@srameshr did you update the Column.js on line 117 with my new _keyExtractor function?

I can do some more tests later to see if there may be a problem with updating the bricks configuration on the fly. Maybe there is an point in time when old and new configuration exists at the same time somewhere inside...

srameshr commented 7 years ago

@soukupl Tried it. Its still the same. I don't know whats the problem here. Could you try it? To avoid that problem im refreshing the whole screen with:

this.props.navigator.resetTo({ screen: 'HomeScreen', animated: false });

To try it, your masonry should look like this:


refreshContent() {
  this.setState({ content:  {
    key: '1',
    id: '1',
    uri: 'http://name.com/hello.jpg',
    data: {}
  },
{
    key: '2',
    id: '2',
    uri: 'http://name.com/world.jpg',
    data: {}
  }})
}
<Masonry
          bricks={this.state.contents}
          columns={this.state.columns}
          sorted
          customImageComponent={FastImage}
          customImageProps={fastProps}
          refreshControl={
            <RefreshControl
              refreshing={this.state.refreshing}
              onRefresh={this.refreshContent.bind(this)}
            />
          }
        />

And in the ListView component of Masonry.js just pass this.props like this:

<ListView
         contentContainerStyle={styles.masonry__container}
         dataSource={this.state.dataSource}
         enableEmptySections
         {...this.props}
         renderRow={(data, sectionId, rowID) =>
           <Column
             data={data}
             columns={this.props.columns}
             parentDimensions={this.state.dimensions}
             imageContainerStyle={this.props.imageContainerStyle}
             customImageComponent={this.props.customImageComponent}
             customImageProps={this.props.customImageProps}
             key={`RN-MASONRY-COLUMN-${rowID}`}/> }
       />
srameshr commented 7 years ago

Did anyone try this?

srameshr commented 7 years ago

@brh55 This issue still exists as I showed you last time.

brh55 commented 7 years ago

@srameshr this exist with the hot fix we did for your project. shouldn't exist in the current branch, this is tied to some caching issues along with being nested in a navigator. It's going to take a bit of investigation to figure what's going on

srameshr commented 7 years ago

@brh55 No, it exists without the hot fix. Just try to do something like pull to refresh on Masonry. All the duplicate warnings show up.

brh55 commented 7 years ago

@srameshr That is due to dynamic data being added most likely and causing duplicated data, created a separate issue to handle this. #61

@soukupl 's PR fixes this bug exactly, since it was caused from a improper key extractor function