react-native-hold-menu icon indicating copy to clipboard operation
react-native-hold-menu copied to clipboard

Doesn't expire cached items prop with arrow functions

Open kesha-antonov opened this issue 3 years ago • 6 comments

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

  • React: 18.1.0
  • React Native: 0.70.5
  • React Native Reanimated: 2.13.0
  • React Native Hold Menu: latest
  • Expo: 47.0.6

kesha-antonov avatar Nov 20 '22 18:11 kesha-antonov

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;
     }

kesha-antonov avatar Nov 20 '22 19:11 kesha-antonov

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

AlexSirenko avatar Nov 26 '22 12:11 AlexSirenko

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

How it's related to caching problem?

kesha-antonov avatar Nov 26 '22 13:11 kesha-antonov

@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>
)

AlexSirenko avatar Nov 26 '22 14:11 AlexSirenko

@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.

kesha-antonov avatar Nov 26 '22 14:11 kesha-antonov

@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

NoodleOfDeath avatar Jul 04 '23 18:07 NoodleOfDeath