Bugfix - FlatList keyExtractor for multicolumn layout

[p-syche]

Summary:

The FlatList keyExtractor for the case of multicolumn lists was causing remounts of all items, in case a single item was removed.
This bug was caused by the fact that the item received by the keyExtractor in case of multicolumn lists, is not an ITEM, but a ROW. This can be seen if you console.log(items) in FlatList's keyExtractor in case of multicolumn layout.
If you check this in the RNTester app, in case of multicolumn layout you will see a following item:

[
    {
        "key": "88",
        "pressed": false,
        "text": "Lorem ipsum dolor sit amet,[...]",
        "title": "Item 88"
    },
    {
        "key": "89",
        "pressed": false,
        "text": "Lorem ipsum dolor sit amet,[...]",
         "title": "Item 89"
    }
]

While in case of single column you will see the following:

{
    "key": "585", 
    "pressed": false, 
    "text": 
    "Lorem ipsum dolor sit amet, [...]",
    "title": "Item 585"
}

Why is this important?

In case of multicolumn layout and a functionality where items can be removed (for example because of a user action), ALL items are unmounted and mounted again. This is caused by how the keyExtractor is built.

Example

The following expo snack shows the unmount/mounting issue in a multicolumn layout:
https://snack.expo.dev/@p-syche/multicolumn
You can remove the numcolumns={3} and remove items in a basic list to see how items are unmounted correctly.
In case of single column only the items coming into the view are mounted.

I created a repo with the same Expo Snack and added a patch for FlatList to demonstrate the bug being fixed:
https://github.com/p-syche/multicolumn-RN-patch/

Changelog:

[GENERAL] [FIXED] - Fixed keyExtractor for multicolumn FlatList.

Test Plan:

I added two buttons to the RNTester app, which let the user remove the 3rd or the 5th element in the FlatList. Pressing either of these buttons we can check that the changed keyExtractor works as expected and does not cause unmounting of all items in the list.

Point for discussion

I was thinking about adding a new prop to FlatList, something like keyExtractorForRow, which would be used in multicolumn layout. This prop would return the array of items for a given index (row).

The solution I implemented is simpler and I don't think it has any downsides, so I abandoned the idea for the new prop.
I'd be very glad to hear your opinions on both solutions.

[NickGerleman]

Given we are no longer calling the keyExtractor for individual items, shifting position now causes different items to now potentially share the same key.

[p-syche]

@NickGerleman the changes in this PR concern multicolumn FlatList only. In the case of multicolumn FlatList, what the keyExtractor receives is the entire row, which is a static wrapper.
I recorded an android emulator with a patched app from this repo: https://github.com/p-syche/multicolumn-RN-patch/
In this repo item IDs are used to generate item keys and in item titles. You can see that when an item is removed everything is working correctly:

Nagranie.ekranu.Feb.29.o.09.55.mp4

[NickGerleman]

Before, when we are generating a key for a row, was the key based on the combined items of the row? Reading the code, it seems like that was the intent.

So, e.g. we would remount if the list of items in the row changed, and that is the issue?

[p-syche]

So, e.g. we would remount if the list of items in the row changed, and that is the issue?

Yes, the entire list would remount - which is unnecessary. The existing implementation works, but is not performant and causes visible problems in case of big lists. We observed this performance issue in a real application in production.

[NickGerleman]

The potential problem I am seeing here, is that we are now reusing keys for a previous row. Wouldn't that allow restoring incorrect state to some existing item, if the row key is now the same as a previous row, in a different state?

[p-syche]

@NickGerleman I'm sorry I don't understand your question :( could you clarify what you mean by:

if the row key is now the same as a previous row, in a different state?

[NickGerleman]

React uses keys to map the identity of items, after rendering, to e.g. keep state associated with a rendered component.

Before, when we remove a single item, instead of a row, all of the keys change, because the identity of every row changes (because items are shifted by one). That causes this unmount issue.

But, if we remove that identity, and only ever key based on position, a row may now share a key as some previously completely different row. So, it seems like we defeat the purpose of keying items, and being able to correlate state between them.

It's not how the component works today, but this kind of feels like the scenario where an ideal component would organize cells using layout, where the React children list can still be flat, and we can key each item individually (and keep identities stable on item changed), while having row/column be a layout-level concept. Though otherwise, I'm not sure the best practices around compounding keys like this, and if remount is the correct behavior.

@rickhanlonii do you know the best practice here?

[p-syche]

Hello @NickGerleman ,

Thank you for your detailed answer 👍
I think I haven't described well enough the issue that this PR is trying to solve. I'll quote two of your sentences:

React uses keys to map the identity of items, after rendering, to e.g. keep state associated with a rendered component.

--> Yes, of course 👍 This PR does not change that.

Before, when we remove a single item, instead of a row, all of the keys change, because the identity of every row changes (because items are shifted by one). That causes this unmount issue.

--> Yes, that is what happens, but it should not. When you remove an item from a list that is not in a multicolumn layout - all keys do not change - why should they change in a multicolumn layout?

I think the most important point here is that by changing the keyExtractor for multicolumn layouts, we're not touching the keys of particular elements. Every item still has it's own key, even if this item is in a different row. You can see that in log examples I put in the PR description.

[NickGerleman]

@p-syche here is a repro, for where not using keyExtractor can cause state to be incorrectly associated. See how after we remove "c", it's state gets transferred to "d". This happens after your change, but not before.

Screen.Recording.2024-03-28.at.3.55.12.PM.mov

/**
 * @flow strict-local
 * @format
 */

import * as React from 'react';
import {useCallback, useState} from 'react';
import {FlatList, Pressable, StyleSheet, Text, View} from 'react-native';

type ListCellProps = {
  item: string,
  ...
};

function ListCell({item}: ListCellProps): React.Node {
  const [isPressed, setIsPressed] = useState(false);

  return (
    <Pressable
      style={[styles.listCell, {backgroundColor: isPressed ? 'blue' : 'red'}]}
      onPress={useCallback(() => setIsPressed(!isPressed), [isPressed])}>
      <Text>{item}</Text>
    </Pressable>
  );
}

const MemoListCell = React.memo(ListCell);


function keyExtractor(item: string, index: number): string {
  return item;
}

export default function Playground(): React.Node {
  const [items, setItems] = useState(['a', 'b', 'c', 'd', 'e', 'f']);

  return (
    <View>
      <Pressable
        onPress={useCallback(
          () => setItems(items.filter(i => i !== 'c')),
          [items],
        )}>
        <Text>Remove C</Text>
      </Pressable>
      <FlatList
        numColumns={2}
        data={items}
        renderItem={useCallback(
          ({item}) => (
            <MemoListCell item={item} />
          ),
          [],
        )}
        keyExtractor={keyExtractor}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  listCell: {
    height: 50,
    flex: 1,
  },
});

[p-syche]

@NickGerleman thank you for the reproduction 🙌 I see the issue now.

Do you agree however that the current implementation, causing remounts, should be changed? Or do you think it should stay as is?

This is definitely still a bug because without any changes all state is wiped after unmount/remount, I'm attaching a video of what I mean.

flatlist_loosing_state.mp4

[p-syche]

Hello again @NickGerleman :)

I have just updated this PR to include changes in how the key is built for items in case of multicolumn layout.
Before - the item index was used for the item key. I suggest we use a different strategy:
const key = keyExtractor(it, index * cols + kk); (changed here )

With this change, items keep their state correctly, unless they change rows. You can see that on the screen recording below:

keep_most_of_state.mp4

Please let me know how you feel about this updated solution.