From 2431e1c9a16675530e247e2f9f0be94f136eaee9 Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Sun, 25 Mar 2018 12:33:11 -0700 Subject: [PATCH] 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 --- .../src/__tests__/NavigationContainer-test.js | 128 +++++++++++++----- .../NavigationContainer-test.js.snap | 13 ++ .../src/createNavigationContainer.js | 33 ++++- .../__tests__/StackNavigator-test.js | 5 + .../react-navigation/src/utils/docsUrl.js | 3 + 5 files changed, 143 insertions(+), 39 deletions(-) create mode 100644 packages/react-navigation/src/__tests__/__snapshots__/NavigationContainer-test.js.snap create mode 100644 packages/react-navigation/src/utils/docsUrl.js diff --git a/packages/react-navigation/src/__tests__/NavigationContainer-test.js b/packages/react-navigation/src/__tests__/NavigationContainer-test.js index 98b1a62e..2aee356d 100644 --- a/packages/react-navigation/src/__tests__/NavigationContainer-test.js +++ b/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 = () =>
; -const BarScreen = () =>
; -const BazScreen = () =>
; -const CarScreen = () =>
; -const DogScreen = () =>
; -const ElkScreen = () =>
; -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 = () =>
; + const BarScreen = () =>
; + const BazScreen = () =>
; + const CarScreen = () =>
; + const DogScreen = () =>
; + const ElkScreen = () =>
; + 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 ; + } + } + + class RootScreen extends React.Component { + render() { + return ( + + + + ); + } + } + + const ChildNavigator = createStackNavigator({ + Child: BlankScreen, + }); + + const RootStack = createStackNavigator({ + Root: RootScreen, + }); + + renderer.create().toJSON(); + expect(spy).toMatchSnapshot(); + }); + }); + }); }); diff --git a/packages/react-navigation/src/__tests__/__snapshots__/NavigationContainer-test.js.snap b/packages/react-navigation/src/__tests__/__snapshots__/NavigationContainer-test.js.snap new file mode 100644 index 00000000..507aa4dc --- /dev/null +++ b/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", + ], + ], + }, +} +`; diff --git a/packages/react-navigation/src/createNavigationContainer.js b/packages/react-navigation/src/createNavigationContainer.js index baa0dea0..bdf7cd79 100644 --- a/packages/react-navigation/src/createNavigationContainer.js +++ b/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 diff --git a/packages/react-navigation/src/navigators/__tests__/StackNavigator-test.js b/packages/react-navigation/src/navigators/__tests__/StackNavigator-test.js index 0c3bfeea..bbb032cf 100644 --- a/packages/react-navigation/src/navigators/__tests__/StackNavigator-test.js +++ b/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().toJSON(); diff --git a/packages/react-navigation/src/utils/docsUrl.js b/packages/react-navigation/src/utils/docsUrl.js new file mode 100644 index 00000000..694f5b3e --- /dev/null +++ b/packages/react-navigation/src/utils/docsUrl.js @@ -0,0 +1,3 @@ +export default function docsUrl(path) { + return `https://v2.reactnavigation.org/docs/${path}`; +}