enesozturk / react-native-hold-menu

📱 A performant, easy to use hold to open context menu for React Native powered by Reanimated 🚀
https://enesozturk.github.io/react-native-hold-menu/
MIT License
1.4k stars 91 forks source link

If menu items do not have unique names keys are not updating #105

Open NoodleOfDeath opened 1 year ago

NoodleOfDeath commented 1 year ago

Describe the bug Because this uses nanoid instead of allowing the developer to specify their own key values, sometimes the menu onPress does not update when it should.

To Reproduce Steps to reproduce the behavior:

  1. Add HoldMenuProvider at the top of stack.
  2. Make a list of HoldItem components with items that differ only by the onPress property but have the same titles and icons for each item.
  3. yarn start --resetCache
  4. Open app. Try long pressing a few items back and forth and occasionally the onPress function will get called for the wrong item rather than the one that was long pressed.

Expected behavior The correct onPress being called for the hold item being long pressed.

Actual behavior Occasionally the wrong onPress is called for the corresponding hold item. Adding an item with text that is unique across all hold items fixes the problem, but I don't want to make them unique other than the onPress functions.

import React from 'react';
import {
  SafeAreaView,
  Text,
  View,
} from 'react-native';

import { HoldItem } from 'react-native-hold-menu';

function Test() {
  const titles = ['one', 'two', 'three'];
  return (
    <View style={ { rowGap: 10 } }>
      {titles.map((title) => (
        <HoldItem
          key={ title }
          items={ [{
            onPress: () => alert(title),
            text: 'share',
          }] }>
          <Text style={ {
            backgroundColor: '#eee',
            padding: 10,
          } }>
            {title}
          </Text>
        </HoldItem>
      ))}
    </View>
  );
}

export function TestScreen() {
  return (
    <SafeAreaView style={ { flex: 1 } }>
      <View style={ { padding: 24 } }>
        <Test />
      </View>
    </SafeAreaView>
  );
}

Screenshots If applicable, add screenshots to help explain your problem.

Not working as expected. https://github.com/enesozturk/react-native-hold-menu/assets/14790443/88583b0d-a8eb-49bc-8bf3-2b22329452f9

Oddly it works when make text unique for each item

 items={ [{
  onPress: () => alert(title),
  text: `share-${title}`,
 }] }>

https://github.com/enesozturk/react-native-hold-menu/assets/14790443/c0643c27-89a2-4be0-b0de-06588bf5738a

Package versions

Additional context Add any other context about the problem here.

NoodleOfDeath commented 1 year ago

~Can I recommend instead of using a nanoid a timebased uuid like uuid.v1 be used? this solved my issue when i replaced nanoidi with new Date().toString()~

Correction -- even using a date string doesnt change it, not a memoized version of the items. only fix is to make the titles unique as shown in screenrecordings

NoodleOfDeath commented 1 year ago

Can we add a key or id property t MenuItemProps? I think the react hooks are not recomputing the items because they don't have anything that differs other than the onPress function

Also, line 91 of HoldItem.tsx has no reactive dependencies, so does that mean it is only computed exactly once on mount?

NoodleOfDeath commented 1 year ago

A HAH! I found the culprit. Line 14 of MenuItems.tsx! Never ever use an index as a key! Adding a key property to MenuItemProps and passing that as the key for MenuItem on line 14 fixes this problem!