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

Doesn't expire cached items prop with arrow functions #86

Open kesha-antonov opened 1 year ago

kesha-antonov commented 1 year ago

Describe the bug Doesn't expire cached items prop with functions

To Reproduce

Pass function to onPress in items Even though you can pass updated items - it's cache on the lib side

const holdItemMenuItems = (
  [
    {
      icon: 'copy-outline',
      text: 'Скопировать',
      onPress: () => {
        console.log('holdItemMenuItems text', text)
        Clipboard.setString(text)
      },
    },
  ]
)

Expected behavior It should call items with new function passed

Actual behavior Caches function forever

Screenshots

https://user-images.githubusercontent.com/11584712/202919893-6302c8c8-cbef-4736-bd9b-202d597e5f60.mp4

Package versions

kesha-antonov commented 1 year ago

This is my temp fix:

patches/react-native-hold-menu+0.1.5.patch
diff --git a/node_modules/react-native-hold-menu/src/utils/validations.ts b/node_modules/react-native-hold-menu/src/utils/validations.ts
index 856a61c..e95d717 100644
--- a/node_modules/react-native-hold-menu/src/utils/validations.ts
+++ b/node_modules/react-native-hold-menu/src/utils/validations.ts
@@ -12,8 +12,10 @@ function fieldAreSame(obj1: MenuItemProps, obj2: MenuItemProps) {
     const val2 = obj2[key];

     if (val1 !== val2) {
-      if (typeof val1 === 'function' && typeof val2 === 'function')
-        return val1.toString() === val2.toString();
+      // FIXME: ARROW FUNCTIONS ALWAYS EQUAL
+      // if (typeof val1 === 'function' && typeof val2 === 'function') {
+      //   return val1.toString() === val2.toString();
+      // }
       return false;
     }
AlexSirenko commented 1 year ago

@kesha-antonov check out this https://enesozturk.github.io/react-native-hold-menu/docs/props#actionparams

kesha-antonov commented 1 year ago

@kesha-antonov check out this https://enesozturk.github.io/react-native-hold-menu/docs/props#actionparams

How it's related to caching problem?

AlexSirenko commented 1 year ago

@kesha-antonov I don't know why caching worked that way, but if you want to pass some changing variable in press function (state, prop, or smth that is changed) you should use actionParams. So your case should be like that:

const holdItemMenuItems = (
  [
    {
      icon: 'copy-outline',
      text: 'Скопировать',
      onPress: (textParam: string) => {
        console.log('holdItemMenuItems text', textParam)
        Clipboard.setString(textParam)
      },
    },
  ]
)

// there some code

return (
// some jsx
<HoldMenu items={holdItemMenuItems} actionParams={{
// there should go array of params that will be passed to onPress callback.
'Скопировать': [text]
}}>
// some jsx children
</HoldMenu>
)
kesha-antonov commented 1 year ago

@kesha-antonov I don't know why caching worked that way, but if you want to pass some changing variable in press function (state, prop, or smth that is changed) you should use actionParams.

So your case should be like that:


const holdItemMenuItems = (

  [

    {

      icon: 'copy-outline',

      text: 'Скопировать',

      onPress: (textParam: string) => {

        console.log('holdItemMenuItems text', textParam)

        Clipboard.setString(textParam)

      },

    },

  ]

)

// there some code

return (

// some jsx

<HoldMenu items={holdItemMenuItems} actionParams={{

// there should go array of params that will be passed to onPress callback.

'Скопировать': [text]

}}>

// some jsx children

</HoldMenu>

)

It's not straightforward and looks more like a bug than a feature. I passed new props and I expect it to be used in a react component. I think here caching should be reconsidered for functions.

NoodleOfDeath commented 1 year ago

@kesha-antonov it's a key reactivity issue. they use the index as the key, so react doesnt know that the menu item needs to be recomputed. Rule of thumb, never use an index as your key when mapping components; or expose this to the developer so they can explicitly tell react when components needs to be recomputed

Here is a fix that only recomputes menu items when you want them to (like any mapping of components) rather than every single tick with your temp fix:

https://github.com/enesozturk/react-native-hold-menu/issues/106