instea / react-native-popup-menu

Popup menu component for React Native
ISC License
1.48k stars 248 forks source link

Exiting the app, then opening throws "multiple instances" warning #144

Open pmiddend opened 5 years ago

pmiddend commented 5 years ago

I've got my MenuProvider set up as the root node of my application, just like the Readme says. However, when I close my app using the hardware "Home" button and then reopen it, I get a message saying I shouldn't need more than one MenuProvider instance.

Is using skipInstanceCheck a good idea in this case? Why does the error/warning occur?

sodik82 commented 5 years ago

Hm, looks strange to me. If you get the warning, the MenuProvider needs to be mounted at least twice.

What are the details? Is it happening on both platforms? What version of popup-menu and RN do you use?

If it is good idea? Well, I believe it should not happen :) Therefore I can't recommend it but it is an option :) Why the warning? Lot of people put Providers directly on the top of PopupMenus e.g. in lists (I believe one reason can be that they just copy paste our "trivial" example applications) and then complain that menu is overlapped by other list items.... And the solution to that is to use only one Provider (as stated in README and docs).

pmiddend commented 5 years ago

@sodik82 I'm using 0.15.0 of react-native-popup-menu and 0.57.8 of react-native itself. The setup is pretty simple, I have MenuProvider at the /very/ root of my hierarchy. Sadly, I can only test android.

sodik82 commented 5 years ago

I tried our Example.js with plain RN on both android and ios simulator without any warning in dev mode.

pmiddend commented 5 years ago

I realized that you can actually look at the code in question, it's in the bottom here:

https://github.com/pmiddend/piggybudget/blob/master/src/App.tsx

So you see anything suspicious?

sodik82 commented 5 years ago

I can confirm that I can see the issue when I try your app. Funny think is that when I "start" the app again, it resets its state - it does not happen with my example app. Logs show

02-09 20:26:35.423 12335 12335 D ReactNative: ReactInstanceManager.attachRootViewToInstance()
02-09 20:26:35.425 12335 12433 I ReactNativeJS: Running application "piggybudget" with appParams: {"rootTag":61}. __DEV__ === true, development-level warning are ON, performance optimizations are OFF
02-09 20:26:35.436 12335 12433 W ReactNativeJS: In most cases you should not have more MenuProviders in your app (see API documentation). In other cases use skipInstanceCheck prop.
02-09 20:26:35.968 12335 12335 I ReactNative: [GESTURE HANDLER] Initialize gesture handler for root view com.swmansion.gesturehandler.react.RNGestureHandlerEnabledRootView{67bfcff V.E...... ......ID 0,0-1080,2043 #3d}

Your code looks fine. Only suspect is native code (maybe gesture handler??). I don't expect any troubles with your setup though.

sodik82 commented 5 years ago

there is a extra step for android installation in https://kmagiera.github.io/react-native-gesture-handler/docs/getting-started.html#android . Might be really the suspect.

pmiddend commented 5 years ago

Thanks for looking it up, I'll try.

pmiddend commented 5 years ago

Hm, no, that's something I already did apparently.

sodik82 commented 5 years ago

do you know the cause?

creambyemute commented 5 years ago

@sodik82 It's not because of the react-native-gesture-handler I believe. Because I'm not using that (still using React-Navigation V2) and experience the same problem when minimizing the app and reopening it.

It happens when resuming via homescreen --> click on app icon instead of resuming via the Recent Activities menu.

Same setup as @pmiddend apart from not using gesture-handler.

sodik82 commented 5 years ago

still quite funny. it looks like MenuProvider is not unmounted but new instance must have been created within the same "JS engine". Otherwise our internal counter (for this warning) can't be high.

creambyemute commented 5 years ago

Some more info :-).

  1. This doesn't happen on iOS. (at least I never saw that error message)
  2. I only ever saw this behaviour on Android and only when resuming via AppIcon instead of recent activities.
  3. Maybe has to do with what is set as android:launchmode=default|singleTop|singleTask|singleInstance in the AndroidManifest.xml? 3.1 I changed my launchMode from default to singleTop on friday to reach consistency to iOS App behaviour when launching/resuming via AppIcon instead of Recent Activities menu. The warning has since then disappeared.

As much as I understood while quickly diving into android:launchMode=default: If your app is backgrounded and could be resumed, it'll do that when resuming via recent activities menu. When starting/resuming via the AppIcon the intent sent will be a different one and it'll create a new instance and throw away the old one. <-- I assume this is exactly where the warning comes from.

singleTop on the other hand will if somehow possible resume the already existing activity instead of creating a new instance, so exactly what the Recent Activities menu does and what iOS does.

jordenchang55 commented 5 years ago

If I use intent/url to open app, this warning will always show. I follow the instruction on ReactNative to set the launchMode to singleTask

Hassanjankhan commented 10 months ago

WARN In most cases you should not have more MenuProviders in your app (see API documentation). In other cases use skipInstanceCheck prop.

not open menu any fixes