search

nandorojo / zeego

Menus for React (Native) done right.
https://zeego.dev
MIT License
1.45k stars 42 forks source link

Catch upstream crash #29

Closed xmflsct closed 1 year ago

xmflsct commented 1 year ago

This is great library taking care of both iOS and Android! After switching to this library, I start to notice a new app crash caused by the upstream react-native-ios-context-menu. I still yet to find out under what circumstances such crash would happen, but this is the line where things go wrong https://github.com/dominicstop/react-native-ios-context-menu/blob/master/src/functions/Helpers.ts#L80. I guess we can catch the error softly to prevent hard crash in this library? Thanks!

nandorojo commented 1 year ago

Interesting, I’ve never seen that. Is it possible you aren’t passing a single native element as the trigger or something?

xmflsct commented 1 year ago

You mean for <*.Trigger> right? Just checked, all triggers include a single React component which may have conditional children rendering inside those components, though these children won't be empty.

nandorojo commented 1 year ago

Oh, could this be from newer React Native versions with strict mode on? I know that findNodeHandle is deprecated...

xmflsct commented 1 year ago

Yeah but in principle findNodeHandle would trigger dev mode warning only, right? I have refactored the menu a bit based on the v1 update, will see if users are still getting such crashes. I myself cannot reproduce it thus not sure how to handle it further. https://github.com/tooot-app/app/issues/644

xmflsct commented 1 year ago

Also Sentry logs show that, it only crashes for iOS 13 users, if it is helpful.

nandorojo commented 1 year ago

That certainly adds context, since iOS 14 I believe is the first to support context menus as far as I remember, and it falls back otherwise. Maybe the upstream lib should set iOS 14 as a minimum deployment target? Alternatively, could we reproduce it there with iOS 13 and open an issue?

xmflsct commented 1 year ago

Closing this as I have applied a temp patch to the upstream lib, and hoping more users will move off iOS 13 soon. For anyone running into this issue, here is the patch https://github.com/tooot-app/app/commit/18ad22302d736edca2c33b450837c338985e4013.