Skip to content

Commit

Permalink
navReducer: Use getNavigationRoutes.
Browse files Browse the repository at this point in the history
Also add a comment, with Greg's description [1] of what's going on here.

We have this handy function for grabbing what we want from the
navigation state, so we might as well use it. When we use this
function, the navigation state is gotten from `NavigationService`,
not from Redux, which moves us closer to zulip#3804.

To be more rigorous, though, we wouldn't want this to stay this way
in `navReduce` forever: reducers are supposed to be pure functions
of state and actions. But the incorrectness isn't obviously harmful
here, and we're about to transplant this code somewhere else, and
it's helpful to separate a small tweak like this from the commit
where we move the code.

[1] zulip#4274 (comment)
  • Loading branch information
chrisbobbe committed Oct 20, 2020
1 parent 2075651 commit e99755c
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 2 deletions.
7 changes: 7 additions & 0 deletions src/nav/__tests__/navReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,17 @@ import deepFreeze from 'deep-freeze';

import { INITIAL_FETCH_COMPLETE } from '../../actionConstants';
import navReducer, { getStateForRoute } from '../navReducer';
import NavigationService from '../NavigationService';

describe('navReducer', () => {
describe('INITIAL_FETCH_COMPLETE', () => {
test('do not mutate navigation state if already at the same route', () => {
NavigationService.getState = jest.fn().mockReturnValue(
deepFreeze({
index: 0,
routes: [{ routeName: 'main' }],
}),
);
const prevState = getStateForRoute('main');

const action = deepFreeze({
Expand Down
12 changes: 11 additions & 1 deletion src/nav/navReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import type { NavigationAction } from 'react-navigation';

import type { NavigationState, Action } from '../types';
import { getNavigationRoutes } from './navSelectors';
import AppNavigator from './AppNavigator';
import { INITIAL_FETCH_COMPLETE } from '../actionConstants';

Expand Down Expand Up @@ -29,7 +30,16 @@ export const initialState = getStateForRoute('loading');
export default (state: NavigationState = initialState, action: Action): NavigationState => {
switch (action.type) {
case INITIAL_FETCH_COMPLETE:
return state.routes[0].routeName === 'main' ? state : getStateForRoute('main');
// If we're anywhere in the normal UI of the app, then remain
// where we are. Only reset the nav state if we're elsewhere,
// and in that case, go to the main screen.
//
// TODO: "elsewhere" is probably just a way of saying "on the
// loading screen", but we're not sure. We could adjust the
// conditional accordingly, if we found out we're not depending on
// the more general condition; see
// https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875
return getNavigationRoutes()[0].routeName === 'main' ? state : getStateForRoute('main');

default: {
// The `react-navigation` libdef says this only takes a NavigationAction,
Expand Down
2 changes: 1 addition & 1 deletion src/nav/navSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import NavigationService from './NavigationService';

export const getNavState = (): NavigationState => NavigationService.getState();

const getNavigationRoutes = () => getNavState().routes;
export const getNavigationRoutes = () => getNavState().routes;

const getNavigationIndex = () => getNavState().index;

Expand Down

0 comments on commit e99755c

Please sign in to comment.