wix / react-native-navigation

A complete native navigation solution for React Native
https://wix.github.io/react-native-navigation/
MIT License
13.04k stars 2.67k forks source link

[V6] Async handler support for AppLaunched event #6168

Closed 39otrebla closed 4 years ago

39otrebla commented 4 years ago

Issue Description

Preamble: this library is outstanding, thank you for the great work.

That said, I'm facing issues implementing what I believe is a quite common scenario. We faced it many times with v1 and v2 and we've never really been able to understand what really works (i.e. no crash in release) and what just partially works (i.e. some AppRegistry crashes in production). Now we are facing it with v6, though I believe we've done all the integration process correctly. But who knows... maybe we are missing some points. So I hope this issue will help to clarify this once forever, and for everyone.

The case is performing async operations within the appLaunched event, before calling setRoot. In particular, if you need to implement conditional roots (as mentioned in the v6 documentation here and here), you end up performing async operations to know whether the user is logged, either by performing network operations, reading from AsyncStorge or by awaiting ReduxPersist's rehydration. And I see in this example that you are indeed using an async handler.

Though, async handler crashes 100% times with the following error: Module AppRegistry is not a registered callable module (calling runApplication). And, if I dismiss the RedBox, our views are perfectly rendered... so there seems to be an issue in (kind of) notifying react native that someone handled the UI rendering. Isn't it?

So the question: are async operations before calling setRoot 100% supported? If yes, which is the proper way of implementing them?

Thank you.

Steps to Reproduce

index.js

// app entry point
import { Navigation } from 'react-native-navigation';

Navigation.events().registerAppLaunchedListener(
  require('./src/app').default
);

src/app.js

[...Redux and ReduxPersist setup...]

const configureStore = () => {
  return new Promise(resolve => {
    let store = createStore(rootReducer, undefined, enhancers);

    const persistor = persistStore(store, null, () => {
      resolve(store);
    });
  });
};

// `appLaunched` event handler
export default async () => {
  // we need to wait rehydration
  const store = await configureStore();
  registerScreens(store);

  // since store is filled, `userIsLogged` can call `store.getState()`
  // and figure out if user is logged
  const isLogged = userIsLogged(store);

  Navigation.setDefaultOptions(NavigationHelper.defaultOptions);

  const root = isLogged ? mainLayout() : authLayout();
  Navigation.setRoot(root);
};

This crashes 100% of the times with the following error: Module AppRegistry is not a registered callable module (calling runApplication)

A closing note: we've also tried to configure the store first, and then register the event passing the rehydrated store as parameter to the handler, so that it doesn't need to be asynchronous. Something like:

configureStore().then(store => {
  Navigation.events().registerAppLaunchedListener(() => {
    app(store); // app is the same handler as above, but synchronous
  });
})

but it doesn't work either, same error.

Environment

Blackfaded commented 4 years ago

A possible solution fur us was to implement an initializing screen from where the main stack gets loaded after all initializations.

const InitializingScreen: FunctionComponentWithOptions = () => {
  StatusBar.setBarStyle('dark-content');

  const { auth } = useStore();

  useEffect(() => {
    init();
  }, []);

  const init = async () => {
    await hydrateStores();

    if (!auth.isAuthenticated) {
      setNoAuthNavigationRoots();
    } else {
      setAuthNavigationRoots();
    }
    SplashScreen.hide();
  };

  return <></>;
};

App.tsx


Navigation.events().registerAppLaunchedListener(() => {
  registerScreens();

  Navigation.setRoot({
    root: {
      component: {
        name: 'Initializing',
      },
    },
  });

  Navigation.setDefaultOptions({
    bottomTabs: {
      visible: false,
    },
  });
});
39otrebla commented 4 years ago

@Blackfaded thank you, that's indeed how we've done it in past. But, the v6 documentation shows an example with an async handler, which of course would be ideal. So I'm asking whether there's an error in the doc or we are just implementing it the wrong way... and anyway, this scenario should be clarified as I believe it is quite common.

Blackfaded commented 4 years ago

The async handler was working for RN 61.5, but it broke with 62.2. So it seems there is a bug. Hope it gets fixed, because the initialization would be more nice in the async handler :)

ghost commented 4 years ago

Same error here...

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe the issue is still relevant, please test on the latest version and report back. Thank you for your contributions.

stale[bot] commented 4 years ago

The issue has been closed for inactivity.

zabojad commented 4 years ago

Hi there!

It's a shame this issue has been closed because we are still experiencing it with RN 0.63.2 and RNN 6.10.1... I've found a dirty fix there.

What happens is that AppRegistry seems not to be loaded early enough.

Guys, @39otrebla, @Blackfaded, @rmartinez-paack did you find any solution for this at your end?

39otrebla commented 4 years ago

@zabojad not yet (at least on RN@0.62.2 and RNN@6.7.2). We've just implemented something similar to this but is all but ideal as the blank screen is quite visible (especially on Android).

zabojad commented 4 years ago

@39otrebla have you tried to "preload" AppRegistry before anything else? By just adding an import and a call to AppRegistry...

Like this for example:

index.js:

import {AppRegistry} from 'react-native';
import { Navigation } from 'react-native-navigation';

console.log('getAppKeys()=',AppRegistry.getAppKeys());

Navigation.events().registerAppLaunchedListener(
  require('./src/app').default
);
39otrebla commented 4 years ago

@zabojad nope. I'll give it a try as soon as I can, I keep you all posted.

39otrebla commented 4 years ago

BTW, we believe that the release where we first implemented this introduced the crash below (only on Android):

java.lang.RuntimeException: 
  at com.reactnativenavigation.viewcontrollers.ViewController.getView (ViewController.java:193)
  at com.reactnativenavigation.viewcontrollers.ChildController.getView (ChildController.java:35)
  at com.reactnativenavigation.anim.StackAnimator$1.onAnimationEnd (StackAnimator.java:60)
  at android.animation.Animator$AnimatorListener.onAnimationEnd (Animator.java:554)
  at android.animation.AnimatorSet.endAnimation (AnimatorSet.java:1301)
  at android.animation.AnimatorSet.doAnimationFrame (AnimatorSet.java:1086)
  at android.animation.AnimationHandler.doAnimationFrame (AnimationHandler.java:146)
  at android.animation.AnimationHandler.access$100 (AnimationHandler.java:37)
  at android.animation.AnimationHandler$1.doFrame (AnimationHandler.java:54)
  at android.view.Choreographer$CallbackRecord.run (Choreographer.java:997)
  at android.view.Choreographer.doCallbacks (Choreographer.java:797)
  at android.view.Choreographer.doFrame (Choreographer.java:728)
  at android.view.Choreographer$FrameDisplayEventReceiver.run (Choreographer.java:984)
  at android.os.Handler.handleCallback (Handler.java:883)
  at android.os.Handler.dispatchMessage (Handler.java:100)
  at android.os.Looper.loop (Looper.java:237)
  at android.app.ActivityThread.main (ActivityThread.java:8125)
  at java.lang.reflect.Method.invoke (Native Method)
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:496)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1100)

It's not terrible (~0,03% of the sessions), but surely something to fix as we are able to achieve 99,99% crash-free with ReactNative on iOS.

In particular, if we'll realize that this is the only way to go for async start-ups, then we should collaborate to fix the crash above.

guyca commented 4 years ago

Hey guys, I've looked into this and couldn't think of why async appLaunchers will cause issues with AppRegistry. As mentioned by @zabojad, this issue happens on projects without RNN. If you suspect RNN is the culprit please provide additional information to support this claim.

Additionally, we use an async handler in the Wix app and I couldn't find these crashes in our monitoring service.

Would be happy to assist in any way possible, feel free to reach out to the maintainers on Discord. Sorry I couldn't provide more help here 👍

zabojad commented 4 years ago

Hey @guyca ! Thank for having looked at this issue. To be honest, I doubt it's a RNN specific issue too as it happens with non RNN projects as well.

guyca commented 4 years ago

Funny thing is... I tried running the example project in Reanimated and encountered this exact same error! 😭

zabojad commented 4 years ago

Have you tried this workaround :p?

guyca commented 4 years ago

@zabojad Just tried it - unfortunately didn't work for me :(

draperunner commented 4 years ago

I am struggling with the same issue after upgrading to RN 0.63.2 (from 0.62.2). The workaround with preloading AppRegistry did not work for me either.

I replaced my app root index.js with the basic RNN Getting Started example and used a dummy App component which is just a View with a Text within. This worked perfectly! So I expect there is something with my RNN screen setup which imports and renders something before my screens are registered. I'll keep you posted if I find out more!

guyca commented 4 years ago

@draperunner That's an interesting observation. Any chance you might have calls to AppRegistry.registerComponent(...);?

draperunner commented 4 years ago

Nope, no references to AppRegistry at all in our code. But I've found that there's something wrong with our dev bundle after upgrading React Native. Our config variables are no longer set. This leads to faulty initializations of certain libraries, like our Auth0 SDK client. After commenting out our Auth0 code, the app launches and the AppRegistry error is gone! So I am pretty certain this config issue is the root of problem, and that it's not related to react-native-navigation directly.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe the issue is still relevant, please test on the latest version and report back. Thank you for your contributions.

stale[bot] commented 4 years ago

The issue has been closed for inactivity.

zabojad commented 4 years ago

Hi there! Not sure if that is an interested piece of information but I just got this issue poping in a new project after having added @react-native-community/netinfo.

As this is a fairly common module, I wondering if you are also using it in the projects where you experience this issue? @guyca @39otrebla @Blackfaded @rmartinez-paack

a-eid commented 3 years ago

you can achieve this with the this code.

let store
const storePromise = configureStore(s).then((store = s))

Navigation.events().registerAppLaunchedListener(async () => {
  await storePromise
  app(store) // app is the same handler as above, but synchronous
})
JanOwiesniak commented 3 years ago

We had a similar situation where the app won't start on some Android devices because we were using a async function to determine which screen to show when the app is launching.

Our code looked like this before (which worked fine on iOS but breaks on some Android):

const lauchApp = async() => {
    Navigation.registerComponent('OnboardingScreen', () => OnboardingScreen);
    Navigation.registerComponent('MainScreen', () => MainScreen);
    // register more screens ...

    const skipOnboarding = await AsyncStorage.getItem('skipOnboarding')
    if (skipOnboarding == null) {
        Navigation.events().registerAppLaunchedListener(() => {
            Navigation.setRoot(routes.onboardingRoot);
        });
    } else {
        Navigation.events().registerAppLaunchedListener(() => {
            Navigation.setRoot(routes.mainRoute);
        });
    }
}

lauchApp();

Passing a async function to registerAppLaunchedListener fixes our issue:

const lauchApp = async() => {
    Navigation.registerComponent('OnboardingScreen', () => OnboardingScreen);
    Navigation.registerComponent('MainScreen', () => MainScreen);
    // register more screens ...

    Navigation.events().registerAppLaunchedListener(async() => {
        const skipOnboarding = await AsyncStorage.getItem('skipOnboarding')
        if (skipOnboarding == null) {
            Navigation.setRoot(routes.onboardingRoot);
        } else {
            Navigation.setRoot(routes.mainRoute);
        }
    })
}

lauchApp();