Warn when users have multiple stateful navigation containers (#3819)

* First pass at warning when users explicitly render nested navigators

* Clean up tests around warnings

* Update comment

* Update comment again

packages/react-navigation/src/__tests__/NavigationContainer-test.js

@@ -1,46 +1,51 @@
 import React from 'react';
 import 'react-native';
+import { StyleSheet, View } from 'react-native';
 
 import renderer from 'react-test-renderer';
 
 import NavigationActions from '../NavigationActions';
-import StackNavigator from '../navigators/createStackNavigator';
-
-const FooScreen = () => <div />;
-const BarScreen = () => <div />;
-const BazScreen = () => <div />;
-const CarScreen = () => <div />;
-const DogScreen = () => <div />;
-const ElkScreen = () => <div />;
-const NavigationContainer = StackNavigator(
-  {
-    foo: {
-      screen: FooScreen,
-    },
-    bar: {
-      screen: BarScreen,
-    },
-    baz: {
-      screen: BazScreen,
-    },
-    car: {
-      screen: CarScreen,
-    },
-    dog: {
-      screen: DogScreen,
-    },
-    elk: {
-      screen: ElkScreen,
-    },
-  },
-  {
-    initialRouteName: 'foo',
-  }
-);
-
-jest.useFakeTimers();
+import createStackNavigator from '../navigators/createStackNavigator';
+import { _TESTING_ONLY_reset_container_count } from '../createNavigationContainer';
 
 describe('NavigationContainer', () => {
+  jest.useFakeTimers();
+  beforeEach(() => {
+    _TESTING_ONLY_reset_container_count();
+  });
+
+  const FooScreen = () => <div />;
+  const BarScreen = () => <div />;
+  const BazScreen = () => <div />;
+  const CarScreen = () => <div />;
+  const DogScreen = () => <div />;
+  const ElkScreen = () => <div />;
+  const NavigationContainer = createStackNavigator(
+    {
+      foo: {
+        screen: FooScreen,
+      },
+      bar: {
+        screen: BarScreen,
+      },
+      baz: {
+        screen: BazScreen,
+      },
+      car: {
+        screen: CarScreen,
+      },
+      dog: {
+        screen: DogScreen,
+      },
+      elk: {
+        screen: ElkScreen,
+      },
+    },
+    {
+      initialRouteName: 'foo',
+    }
+  );
+
   describe('state.nav', () => {
     it("should be preloaded with the router's initial state", () => {
       const navigationContainer = renderer
@@ -198,4 +203,57 @@ describe('NavigationContainer', () => {
       });
     });
   });
+
+  describe('warnings', () => {
+    function spyConsole() {
+      let spy = {};
+
+      beforeEach(() => {
+        spy.console = jest.spyOn(console, 'error').mockImplementation(() => {});
+      });
+
+      afterEach(() => {
+        spy.console.mockRestore();
+      });
+
+      return spy;
+    }
+
+    describe('detached navigators', () => {
+      beforeEach(() => {
+        _TESTING_ONLY_reset_container_count();
+      });
+
+      let spy = spyConsole();
+
+      it('warns when you render more than one navigator explicitly', () => {
+        class BlankScreen extends React.Component {
+          render() {
+            return <View />;
+          }
+        }
+
+        class RootScreen extends React.Component {
+          render() {
+            return (
+              <View>
+                <ChildNavigator />
+              </View>
+            );
+          }
+        }
+
+        const ChildNavigator = createStackNavigator({
+          Child: BlankScreen,
+        });
+
+        const RootStack = createStackNavigator({
+          Root: RootScreen,
+        });
+
+        renderer.create(<RootStack />).toJSON();
+        expect(spy).toMatchSnapshot();
+      });
+    });
+  });
 });

packages/react-navigation/src/__tests__/__snapshots__/NavigationContainer-test.js.snap

@@ -0,0 +1,13 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`NavigationContainer warnings detached navigators warns when you render more than one navigator explicitly 1`] = `
+Object {
+  "console": [MockFunction] {
+    "calls": Array [
+      Array [
+        "You should only render one navigator explicitly in your app, and other navigators should by rendered by including them in that navigator. Full details at: https://v2.reactnavigation.org/docs/common-mistakes.html#explicitly-rendering-more-than-one-navigator",
+      ],
+    ],
+  },
+}
+`;

packages/react-navigation/src/createNavigationContainer.js

@@ -1,11 +1,12 @@
 import React from 'react';
-import withLifecyclePolyfill from 'react-lifecycles-compat';
 import { Linking, AsyncStorage } from 'react-native';
+import withLifecyclePolyfill from 'react-lifecycles-compat';
 
 import { BackHandler } from './PlatformHelpers';
 import NavigationActions from './NavigationActions';
 import addNavigationHelpers from './addNavigationHelpers';
 import invariant from './utils/invariant';
+import docsUrl from './utils/docsUrl';
 
 function isStateful(props) {
   return !props.navigation;
@@ -32,12 +33,22 @@ function validateProps(props) {
   }
 }
 
+// Track the number of stateful container instances. Warn if >0 and not using the
+// detached prop to explicitly acknowledge the behavior. We should deprecated implicit
+// stateful navigation containers in a future release and require a provider style pattern
+// instead in order to eliminate confusion entirely.
+let _statefulContainerCount = 0;
+export function _TESTING_ONLY_reset_container_count() {
+  _statefulContainerCount = 0;
+}
+
 // We keep a global flag to catch errors during the state persistence hydrating scenario.
 // The innermost navigator who catches the error will dispatch a new init action.
 let _reactNavigationIsHydratingState = false;
-// Unfortunate to use global state here, but it seems necessesary for the time being. There seems to
-// be some problems with cascading componentDidCatch handlers. Ideally the inner non-stateful navigator
-// catches the error and re-throws it, to be caught by the top-level stateful navigator.
+// Unfortunate to use global state here, but it seems necessesary for the time
+// being. There seems to be some problems with cascading componentDidCatch
+// handlers. Ideally the inner non-stateful navigator catches the error and
+// re-throws it, to be caught by the top-level stateful navigator.
 
 /**
  * Create an HOC that injects the navigation and manages the navigation state
@@ -186,6 +197,16 @@ export default function createNavigationContainer(Component) {
         return;
       }
 
+      if (__DEV__ && !this.props.detached) {
+        if (_statefulContainerCount > 0) {
+          console.error(
+            `You should only render one navigator explicitly in your app, and other navigators should by rendered by including them in that navigator. Full details at: ${docsUrl(
+              'common-mistakes.html#explicitly-rendering-more-than-one-navigator'
+            )}`
+          );
+        }
+      }
+      _statefulContainerCount++;
       Linking.addEventListener('url', this._handleOpenURL);
 
       const { persistenceKey } = this.props;
@@ -257,6 +278,10 @@ export default function createNavigationContainer(Component) {
       this._isMounted = false;
       Linking.removeEventListener('url', this._handleOpenURL);
       this.subs && this.subs.remove();
+
+      if (this._isStateful()) {
+        _statefulContainerCount--;
+      }
     }
 
     // Per-tick temporary storage for state.nav

packages/react-navigation/src/navigators/__tests__/StackNavigator-test.js

@@ -4,6 +4,7 @@ import renderer from 'react-test-renderer';
 
 import StackNavigator from '../createStackNavigator';
 import withNavigation from '../../views/withNavigation';
+import { _TESTING_ONLY_reset_container_count } from '../../createNavigationContainer';
 
 const styles = StyleSheet.create({
   header: {
@@ -32,6 +33,10 @@ const routeConfig = {
 };
 
 describe('StackNavigator', () => {
+  beforeEach(() => {
+    _TESTING_ONLY_reset_container_count();
+  });
+
   it('renders successfully', () => {
     const MyStackNavigator = StackNavigator(routeConfig);
     const rendered = renderer.create(<MyStackNavigator />).toJSON();

packages/react-navigation/src/utils/docsUrl.js

@@ -0,0 +1,3 @@
+export default function docsUrl(path) {
+  return `https://v2.reactnavigation.org/docs/${path}`;
+}