From 87e88b9b99b4d775de07096d0dac4a5af024a907 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 3 Dec 2021 08:46:38 -1000 Subject: [PATCH 1/4] clean stuff up --- src/libs/Navigation/CustomActions.js | 47 ++++++++++++---------- src/libs/Navigation/Navigation.js | 59 ++++++++++++++++++---------- 2 files changed, 66 insertions(+), 40 deletions(-) diff --git a/src/libs/Navigation/CustomActions.js b/src/libs/Navigation/CustomActions.js index 8cf1d40c2c87..328497866376 100644 --- a/src/libs/Navigation/CustomActions.js +++ b/src/libs/Navigation/CustomActions.js @@ -4,12 +4,13 @@ import { } from '@react-navigation/native'; import lodashGet from 'lodash/get'; import linkingConfig from './linkingConfig'; +import navigationRef from './navigationRef'; /** * Go back to the Main Drawer * @param {Object} navigationRef */ -function navigateBackToRootDrawer(navigationRef) { +function navigateBackToRootDrawer() { let isLeavingNestedDrawerNavigator = false; // This should take us to the first view of the modal's stack navigator @@ -42,43 +43,49 @@ function navigateBackToRootDrawer(navigationRef) { } /** - * In order to create the desired browser navigation behavior on web and mobile web we need to replace any - * type: 'drawer' routes with a type: 'route' so that when pushing to a report screen we never show the - * drawer. We don't want to remove these since we always want the history length to increase by 1 whenever - * we are moving to a new report screen or back to a previous one. This is a workaround since - * react-navigation default behavior for a drawer is to skip pushing history states when navigating to the - * current route + * @param {Object} state + * @returns {Object} + */ +function getParamsFromState(state) { + return lodashGet(state, 'routes[0].state.routes[0].params', {}); +} + +/** + * Special accomodation must be made for navigating to a screen inside a DrawerNavigator (e.g. our ReportScreen). The web/mWeb default behavior when + * calling "navigate()" does not give us the browser history we would expect for a typical web paradigm (e.g. that navigating from one screen another + * should allow us to navigate back to the screen we were on previously). This custom action helps us get around these problems. + * + * More context here: https://github.com/react-navigation/react-navigation/issues/9744 * - * @param {String} screenName - * @param {Object} params * @param {String} path - * @param {Object} navigationRef * @returns {Function} */ -function pushDrawerRoute(screenName, params, path, navigationRef) { +function pushDrawerRoute(path) { return (currentState) => { let state = currentState; + const stateFromPath = getStateFromPath(path, linkingConfig.config); + const newScreenName = lodashGet(stateFromPath, 'routes[0].state.routes[0].name'); + const newScreenParams = getParamsFromState(stateFromPath); + // Avoid the navigation and refocus the report if we're trying to navigate to our active report // We use our RootState as the dispatch's state is relative to the active navigator and might // not contain our active report. const rootState = navigationRef.current.getRootState(); - const activeReportID = lodashGet(rootState, 'routes[0].state.routes[0].params.reportID', ''); + const activeScreenParams = getParamsFromState(rootState); if (state.type !== 'drawer') { - navigateBackToRootDrawer(navigationRef); + navigateBackToRootDrawer(); } - if (activeReportID === params.reportID) { + if (_.isEqual(activeScreenParams, newScreenParams)) { return DrawerActions.closeDrawer(); } // When navigating from non Drawer navigator, get new state for the report and reset the navigation state. if (state.type !== 'drawer') { - state = linkingConfig.getStateFromPath - ? linkingConfig.getStateFromPath(path, linkingConfig.config) - : getStateFromPath(path, linkingConfig.config); + state = getStateFromPath(path, linkingConfig.config); } - const screenRoute = {type: 'route', name: screenName}; + const screenRoute = {type: 'route', name: newScreenName}; const history = _.map([...(state.history || [screenRoute])], () => screenRoute); // Force drawer to close and show the report screen @@ -89,8 +96,8 @@ function pushDrawerRoute(screenName, params, path, navigationRef) { return CommonActions.reset({ ...state, routes: [{ - name: screenName, - params, + name: newScreenName, + params: newScreenParams, }], history, }); diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js index 50613b5ad741..9e0312aef2f1 100644 --- a/src/libs/Navigation/Navigation.js +++ b/src/libs/Navigation/Navigation.js @@ -4,7 +4,6 @@ import {Keyboard} from 'react-native'; import { StackActions, DrawerActions, - createNavigationContainerRef, getPathFromState, } from '@react-navigation/native'; import PropTypes from 'prop-types'; @@ -12,10 +11,10 @@ import Onyx from 'react-native-onyx'; import Log from '../Log'; import linkTo from './linkTo'; import ROUTES from '../../ROUTES'; -import SCREENS from '../../SCREENS'; import CustomActions from './CustomActions'; import ONYXKEYS from '../../ONYXKEYS'; import linkingConfig from './linkingConfig'; +import navigationRef from './navigationRef'; let isLoggedIn = false; Onyx.connect({ @@ -23,8 +22,6 @@ Onyx.connect({ callback: val => isLoggedIn = Boolean(val && val.authToken), }); -const navigationRef = createNavigationContainerRef(); - // This flag indicates that we're trying to deeplink to a report when react-navigation is not fully loaded yet. // If true, this flag will cause the drawer to start in a closed state (which is not the default for small screens) // so it doesn't cover the report we're trying to link to. @@ -38,15 +35,29 @@ function setDidTapNotification() { didTapNotificationBeforeReady = true; } +/** + * @param {String} methodName + * @param {Object} params + * @returns {Boolean} + */ +function canNavigate(methodName, params = {}) { + if (navigationRef.isReady()) { + return true; + } + + Log.hmmm(`[Navigation] ${methodName} failed because navigation ref was not yet ready`, params); + return false; +} + /** * Opens the LHN drawer. * @private */ function openDrawer() { - if (!navigationRef.isReady()) { - Log.hmmm('[Navigation] openDrawer failed because navigation ref was not yet ready'); + if (!canNavigate('openDrawer')) { return; } + navigationRef.current.dispatch(DrawerActions.openDrawer()); Keyboard.dismiss(); } @@ -56,10 +67,10 @@ function openDrawer() { * @private */ function closeDrawer() { - if (!navigationRef.isReady()) { - Log.hmmm('[Navigation] closeDrawer failed because navigation ref was not yet ready'); + if (!canNavigate('closeDrawer')) { return; } + navigationRef.current.dispatch(DrawerActions.closeDrawer()); } @@ -79,8 +90,7 @@ function getDefaultDrawerState(isSmallScreenWidth) { * @param {Boolean} shouldOpenDrawer */ function goBack(shouldOpenDrawer = true) { - if (!navigationRef.isReady()) { - Log.hmmm('[Navigation] goBack failed because navigation ref was not yet ready'); + if (!canNavigate('goBack')) { return; } @@ -95,13 +105,26 @@ function goBack(shouldOpenDrawer = true) { navigationRef.current.goBack(); } +/** + * We navigate to the certains screens with a custom action so that we can preserve the browser history in web. react-navigation does not handle this well + * and only offers a "mobile" navigation paradigm e.g. in order to add a history item onto the browser history stack one would need to use the "push" action. + * However, this is not performant as it would keep stacking ReportScreen instances (which are quite expensive to render). + * We're also looking to see if we have a participants route since those also have a reportID param, but do not have the problem described above and should not use the custom action. + * + * @param {String} route + * @returns {Boolean} + */ +function isDrawerRoute(route) { + const {reportID, isParticipantsRoute} = ROUTES.parseReportRouteParams(route); + return reportID && !isParticipantsRoute; +} + /** * Main navigation method for redirecting to a route. * @param {String} route */ function navigate(route = ROUTES.HOME) { - if (!navigationRef.isReady()) { - Log.hmmm('[Navigation] navigate failed because navigation ref was not yet ready', {route}); + if (!canNavigate('navigate', {route})) { return; } @@ -117,11 +140,8 @@ function navigate(route = ROUTES.HOME) { return; } - // Navigate to the ReportScreen with a custom action so that we can preserve the history. We're looking to see if we - // have a participants route since those should go through linkTo() as they open a different screen. - const {reportID, isParticipantsRoute} = ROUTES.parseReportRouteParams(route); - if (reportID && !isParticipantsRoute) { - navigationRef.current.dispatch(CustomActions.pushDrawerRoute(SCREENS.REPORT, {reportID}, route, navigationRef)); + if (isDrawerRoute(route)) { + navigationRef.current.dispatch(CustomActions.pushDrawerRoute(route)); return; } @@ -134,8 +154,7 @@ function navigate(route = ROUTES.HOME) { * @param {Boolean} [shouldOpenDrawer] */ function dismissModal(shouldOpenDrawer = false) { - if (!navigationRef.isReady()) { - Log.hmmm('[Navigation] dismissModal failed because navigation ref was not yet ready'); + if (!canNavigate('dismissModal')) { return; } @@ -143,7 +162,7 @@ function dismissModal(shouldOpenDrawer = false) { ? shouldOpenDrawer : false; - CustomActions.navigateBackToRootDrawer(navigationRef); + CustomActions.navigateBackToRootDrawer(); if (normalizedShouldOpenDrawer) { openDrawer(); } From 6c71c6665217a755ea95ac7db9ad7ec1206e305c Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 3 Dec 2021 09:41:59 -1000 Subject: [PATCH 2/4] try to improve comments --- src/libs/Navigation/CustomActions.js | 60 +++++++++++++++++++--------- src/libs/Navigation/navigationRef.js | 6 +++ 2 files changed, 47 insertions(+), 19 deletions(-) create mode 100644 src/libs/Navigation/navigationRef.js diff --git a/src/libs/Navigation/CustomActions.js b/src/libs/Navigation/CustomActions.js index 328497866376..b88617b11234 100644 --- a/src/libs/Navigation/CustomActions.js +++ b/src/libs/Navigation/CustomActions.js @@ -50,6 +50,22 @@ function getParamsFromState(state) { return lodashGet(state, 'routes[0].state.routes[0].params', {}); } +/** + * @param {Object} state + * @returns {String} + */ +function getScreenNameFromState(state) { + return lodashGet(state, 'routes[0].state.routes[0].name', ''); +} + +/** + * @returns {Object} + */ +function getActiveState() { + // We use our RootState as the dispatch's state is relative to the active navigator and might not contain our active screen. + return navigationRef.current.getRootState(); +} + /** * Special accomodation must be made for navigating to a screen inside a DrawerNavigator (e.g. our ReportScreen). The web/mWeb default behavior when * calling "navigate()" does not give us the browser history we would expect for a typical web paradigm (e.g. that navigating from one screen another @@ -57,42 +73,48 @@ function getParamsFromState(state) { * * More context here: https://github.com/react-navigation/react-navigation/issues/9744 * - * @param {String} path + * @param {String} route * @returns {Function} */ -function pushDrawerRoute(path) { +function pushDrawerRoute(route) { return (currentState) => { - let state = currentState; - - const stateFromPath = getStateFromPath(path, linkingConfig.config); - const newScreenName = lodashGet(stateFromPath, 'routes[0].state.routes[0].name'); - const newScreenParams = getParamsFromState(stateFromPath); + // Parse the state, name, and params from the new route we want to navigate to + const newStateFromRoute = getStateFromPath(route, linkingConfig.config); + const newScreenName = getScreenNameFromState(newStateFromRoute); + const newScreenParams = getParamsFromState(newStateFromRoute); - // Avoid the navigation and refocus the report if we're trying to navigate to our active report - // We use our RootState as the dispatch's state is relative to the active navigator and might - // not contain our active report. - const rootState = navigationRef.current.getRootState(); - const activeScreenParams = getParamsFromState(rootState); - if (state.type !== 'drawer') { + // When we are navigating away from a non-drawer navigator we need to dismiss any screens pushed onto the main stack. + if (currentState.type !== 'drawer') { navigateBackToRootDrawer(); } - if (_.isEqual(activeScreenParams, newScreenParams)) { + + // If we're trying to navigate to the same screen that is already active there's nothing more to do except close the drawer. + // This prevents unnecessary re-rendering the screen and adding duplicate items to the browser history. + const activeState = getActiveState(); + const activeScreenName = getScreenNameFromState(activeState); + const activeScreenParams = getParamsFromState(activeState); + if (newScreenName === activeScreenName && _.isEqual(activeScreenParams, newScreenParams)) { return DrawerActions.closeDrawer(); } - // When navigating from non Drawer navigator, get new state for the report and reset the navigation state. - if (state.type !== 'drawer') { - state = getStateFromPath(path, linkingConfig.config); + let state = currentState; + + // When navigating from non-Drawer navigator we switch to using the new state generated from the provided route. If we are navigating away from a non-Drawer navigator the + // currentState will not have a history field to use. By using the state from the route we create a "fresh state" that we can use to setup the browser history again. + // Note: A current limitation with this is that navigating "back" won't display the routes we have cleared out e.g. SearchPage and the history effectively gets "reset". + if (currentState.type !== 'drawer') { + state = newStateFromRoute; } const screenRoute = {type: 'route', name: newScreenName}; - const history = _.map([...(state.history || [screenRoute])], () => screenRoute); + const history = _.map(state.history ? [...state.history] : [screenRoute], () => screenRoute); - // Force drawer to close and show the report screen + // Force drawer to close and show history.push({ type: 'drawer', status: 'closed', }); + return CommonActions.reset({ ...state, routes: [{ diff --git a/src/libs/Navigation/navigationRef.js b/src/libs/Navigation/navigationRef.js new file mode 100644 index 000000000000..a5a4fbfabc8a --- /dev/null +++ b/src/libs/Navigation/navigationRef.js @@ -0,0 +1,6 @@ +import { + createNavigationContainerRef, +} from '@react-navigation/native'; + +const navigationRef = createNavigationContainerRef(); +export default navigationRef; From 10be228406f98da23a7bdf81e30055c611e9b35e Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 3 Dec 2021 09:53:27 -1000 Subject: [PATCH 3/4] improve comment --- src/libs/Navigation/CustomActions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/Navigation/CustomActions.js b/src/libs/Navigation/CustomActions.js index b88617b11234..5a4519a95ca6 100644 --- a/src/libs/Navigation/CustomActions.js +++ b/src/libs/Navigation/CustomActions.js @@ -78,12 +78,12 @@ function getActiveState() { */ function pushDrawerRoute(route) { return (currentState) => { - // Parse the state, name, and params from the new route we want to navigate to + // Parse the state, name, and params from the new route we want to navigate to. const newStateFromRoute = getStateFromPath(route, linkingConfig.config); const newScreenName = getScreenNameFromState(newStateFromRoute); const newScreenParams = getParamsFromState(newStateFromRoute); - // When we are navigating away from a non-drawer navigator we need to dismiss any screens pushed onto the main stack. + // When we are navigating away from a non-drawer navigator we need to first dismiss any screens pushed onto the main stack. if (currentState.type !== 'drawer') { navigateBackToRootDrawer(); } From d47e38f3e50d89c3ab183f44f088baf130ceb5f4 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 7 Dec 2021 08:12:24 -1000 Subject: [PATCH 4/4] add note to explain getRouteFromState --- src/libs/Navigation/CustomActions.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/libs/Navigation/CustomActions.js b/src/libs/Navigation/CustomActions.js index 5a4519a95ca6..245d5d7340ba 100644 --- a/src/libs/Navigation/CustomActions.js +++ b/src/libs/Navigation/CustomActions.js @@ -42,12 +42,23 @@ function navigateBackToRootDrawer() { } } +/** + * Extracts the route from state object. Note: In the context where this is used currently the method is dependable. + * However, as our navigation system grows in complexity we may need to revisit this to be sure it is returning the expected route object. + * + * @param {Object} state + * @return {Object} + */ +function getRouteFromState(state) { + return lodashGet(state, 'routes[0].state.routes[0]', {}); +} + /** * @param {Object} state * @returns {Object} */ function getParamsFromState(state) { - return lodashGet(state, 'routes[0].state.routes[0].params', {}); + return getRouteFromState(state).params || {}; } /** @@ -55,7 +66,7 @@ function getParamsFromState(state) { * @returns {String} */ function getScreenNameFromState(state) { - return lodashGet(state, 'routes[0].state.routes[0].name', ''); + return getRouteFromState(state).name || ''; } /**