handle new SwitchRouter history back behaviors #31

slorber · 2019-01-31T10:33:00Z

Hi,

This is a proposal implementation for new SwitchRouter backBehaviors documented in RFC:

https://github.com/react-navigation/rfcs/blob/master/text/0008-back-behaviour-for-tabs.md
react-navigation/rfcs#2

I didn't test it in a real app but the tests are passing and seems ok regarding the RFC, is there any easy setup to do so?

Also note I created a ExampleRouterHelper which makes tests easier to read, by creating a "stateful router", because most of the tests are generally executing one action after the other. If you like this better I can rewrite all the tests this way in another PR (or this one, your choice), or revert to the previous way of testing which involves creating many local state variables. I can also try to apply the same technique when appropriate on other test files by creating a more generic helper.

slorber · 2019-01-31T10:38:28Z

cc @satya164 @ericvicenti @brentvatne

@satya164 your RFC did not set the history option as default backbehavior, should we do this? I've seen this in your comment here react-navigation/rfcs#2 (comment)

brentvatne · 2019-02-03T19:08:42Z

thank you! will review this soon

ericvicenti · 2019-02-04T07:29:25Z

src/routers/SwitchRouter.js

  const resetOnBlur = config.hasOwnProperty('resetOnBlur')
    ? config.resetOnBlur
    : true;
  const initialRouteIndex = order.indexOf(initialRouteName);
+
+  const routeIndexHistory = [initialRouteIndex];


Looks like this is state contained in the router. Routers are currently entirely stateless, in order to make navigation actions predictable and debuggable. The rest of the stuff out in this context is router configuration. (In theory, stateless routers could enable dynamic routers, where the whole router gets swapped out, although we don't really handle that now.)

So I think we should make sure that anything "history" related lives in navigation state, even if it means we need to add an extra key like "routeIndexHistory" for the switch router in this mode.

But also, I think maybe the route history could actually be saved in state.routes, so that the navigation state resembles the stack nav state.

OK. As this state is exposed to users I'll let you choose the key name.

Also I choose the history to have min length = 1 but I could have also made it min length 0,do you have a preference?

Personally I prefer using route.key over index, which would semantically preserve order if the routes changed for some reason. I think routeKeyHistory is a sensible name. Lets make sure to only assign that property when this router mode is used.

agree with @ericvicenti's comments here :)

Hi, I've made the change to add a routeKeyHistory to the state.
As far as I understand, for a switch router the key === name right? as it does not make any sense to navigate to the same screen under different names?

Also, adding this history complicates slightly the code, as there are many places where the next state can be returned. I'm not totally sure to have covered all the edge cases but at least my test is passing. Please tell me what you think

ericvicenti

Thank you for working on this! I'm excited to get you ramped up on core

ericvicenti · 2019-02-04T07:31:25Z

src/routers/SwitchRouter.js

  const resetOnBlur = config.hasOwnProperty('resetOnBlur')
    ? config.resetOnBlur
    : true;
  const initialRouteIndex = order.indexOf(initialRouteName);
+
+  const routeIndexHistory = [initialRouteIndex];


But also, I think maybe the route history could actually be saved in state.routes, so that the navigation state resembles the stack nav state.

ericvicenti · 2019-02-04T07:34:45Z

src/routers/SwitchRouter.js

@@ -177,8 +196,16 @@ export default (routeConfigs, config = {}) => {
      const isBackEligible =
        action.key == null || action.key === activeChildLastState.key;
      if (action.type === NavigationActions.BACK) {
-        if (isBackEligible && shouldBackNavigateToInitialRoute) {
-          activeChildIndex = initialRouteIndex;
+        if (isBackEligible && backBehavior !== 'none') {


these ifs can be flattened, and IMO it is easier to read that way. Redundant else return state can be eliminated.

ericvicenti · 2019-02-04T07:42:52Z

src/routers/__tests__/SwitchRouter-test.js

+// A simple helper that makes it easier to write basic routing tests
+// As we generally want to apply one action after the other,
+// it's often convenient to manipulate a structure that keeps the router state
+class ExampleRouterHelper {


As a general rule I tend to avoid utilities or helpers in my tests, because they make it harder to see what is actually being tested. The rest of the tests in this library may be verbose, but they follow this philosophy.

For example, how often do the new tests check against the initial state? Only once. What if you pass a navigate action in with a null state? The helper makes it impossible to test that case.

That being said, this helper isn't the worst example, and the philosophy is just my opinion. So I'll leave it to Brent and your judgement on this one.

I see. I somehow agree with your philosophy but also I think the helper helps avoiding typos in tests. For example when writing tests with no helper I first made a typo where I passed state2 instead of state.

I'll let you and Brent choose and I'm OK to revert, but we should rather keep tests consistant with each other's

I agree we should be consistent, but we could slowly migrate other tests if the best practice changes. @brentvatne do you have a preference?

I agree with @slorber that it can be a bit tedious to make sure you're threading through the correct state variables. We should pull a class like this out to a helper file, call it something like RouterTestHelper, and have it accept a param for how it should initialize the router. Let's do that in a follow-up PR and change other tests to use it at the same time, and leave this PR with the helper as it is for now.

ok, I'll try to do another PR. I'll do one for SwitchRouter first and then StackRouter. StackRouter has more complicated tests so it will mostly apply to tests of getStateForAction

Sounds good!

(Also I can't help but humbly mention my preference for functions+return objects+closures over ES classes.. const { applyAction, getState } = getRouterTestHelper(router); )

@ericvicenti - i'd be curious to see what that looks like next to the class version

I can give a try in another PR and you compare. I'm fine with both as long as the tests are easy to read

brentvatne · 2019-02-05T01:54:28Z

I didn't test it in a real app but the tests are passing and seems ok regarding the RFC, is there any easy setup to do so?

I updated the example project to SDK32 and added a good way to do test @react-navigation/core changes in it: https://github.com/react-navigation/react-navigation-core#development-workflow - hope that helps! If you rebase this PR on master you'll be able to use it.

@satya164 your RFC did not set the history option as default backbehavior, should we do this? I've seen this in your comment here react-navigation/rfcs#2 (comment)

I think the default should be the current behavior, initialRoute. If we decide at a later date that we want to change that we'll need to do a major version bump of react-navigation

slorber · 2019-02-05T10:44:46Z

src/routers/SwitchRouter.js

  const initialRouteIndex = order.indexOf(initialRouteName);
+  if (initialRouteIndex === -1) {


moved this check closer to initialRouteIndex, seems appropriate

slorber · 2019-02-05T10:54:08Z

I've made the changes required to put the history to navigation state. Not sure exactly what I'm doing so take care reviewing this :D

Will try to check to add an example soon.

Agree we can keep for now the initialRoute, and eventually discuss switching to "history" in 4.0

slorber · 2019-02-05T10:56:14Z

src/routers/__tests__/SwitchRouter-test.js

+        type: NavigationActions.NAVIGATE,
+        routeName: 'C',
+      })
+    ).toMatchObject({ index: 2, routeKeyHistory: ['B', 'A', 'C'] });


just a dev note: here the object is matched "partially" but the array is matched "fully". Not really explicit from jest doc though...

If the state contains ['B', 'A', 'C', 'D'] it will effectively fail because of presence of D

Makes sense that its a shallow match 👌

ericvicenti

Looking great!

ericvicenti · 2019-02-05T20:45:47Z

src/routers/SwitchRouter.js

+            const routeKey =
+              state.routeKeyHistory[state.routeKeyHistory.length - 2];
+            activeChildIndex = order.indexOf(routeKey);
+          } else {


looks like you can flatten this out again 😛

ericvicenti · 2019-02-05T20:49:21Z

src/routers/__tests__/SwitchRouter-test.js

+// A simple helper that makes it easier to write basic routing tests
+// As we generally want to apply one action after the other,
+// it's often convenient to manipulate a structure that keeps the router state
+class ExampleRouterHelper {


Sounds good!

(Also I can't help but humbly mention my preference for functions+return objects+closures over ES classes.. const { applyAction, getState } = getRouterTestHelper(router); )

ericvicenti · 2019-02-05T20:54:15Z

src/routers/__tests__/SwitchRouter-test.js

+        type: NavigationActions.NAVIGATE,
+        routeName: 'C',
+      })
+    ).toMatchObject({ index: 2, routeKeyHistory: ['B', 'A', 'C'] });


Makes sense that its a shallow match 👌

brentvatne

just a couple of small comments, thanks for doing this :D

brentvatne · 2019-02-06T01:06:52Z

src/routers/SwitchRouter.js

@@ -226,7 +263,7 @@ export default (routeConfigs, config = {}) => {
              routes,
              index: activeChildIndex,
            };
-            return getNextState(prevState, nextState);
+            return updateRouteKeyHistory(getNextState(prevState, nextState));


can we move updateRouteKeyHistory into getNextState? afraid of it being too easy to accidentally not update the route key history in the future

brentvatne · 2019-02-06T01:07:03Z

src/routers/SwitchRouter.js

-          ...state,
-          index: activeChildIndex,
-        });
+        return updateRouteKeyHistory(


same as above re: moving to getNextState

brentvatne · 2019-02-06T01:08:09Z

src/routers/__tests__/SwitchRouter-test.js

+// A simple helper that makes it easier to write basic routing tests
+// As we generally want to apply one action after the other,
+// it's often convenient to manipulate a structure that keeps the router state
+class ExampleRouterHelper {


@ericvicenti - i'd be curious to see what that looks like next to the class version

brentvatne · 2019-02-06T01:10:43Z

src/routers/SwitchRouter.js

+          nextRouteKeyHistory = nextRouteKeyHistory.filter(k => k !== keyToAdd); // Deduplicate
+          nextRouteKeyHistory.push(keyToAdd);
+        }
+        if (action.type === NavigationActions.BACK) {


slorber · 2019-02-06T11:06:28Z

src/routers/SwitchRouter.js

-  function getNextState(prevState, possibleNextState) {
-    if (!prevState) {
-      return possibleNextState;
+  function getNextState(action, prevState, possibleNextState) {


@brentvatne I added action to the params list there as it's needed for updateNextStateHistory.

Didn't want to move getNextState inside getStateForActions. Don't really like to have a lot of deeply nested closures whose captured variables are not very "visible", keeping the function outside makes it clearer which are the variable dependencies.

Also put updateNextStateHistory inside getNextState as it's the only place where it's used

slorber · 2019-02-06T11:07:18Z

src/routers/SwitchRouter.js

    }
-
-    return nextState;
+    return updateNextStateHistory(nextState);


I simplified a bit the getNextState original code, it's clearer to have a single return path imho

brentvatne · 2019-02-09T01:50:02Z

nice work @slorber :)

brentvatne · 2019-02-09T02:26:06Z

@slorber - want to open a pr on https://github.com/react-navigation/react-navigation.github.io with info on this change? :)

slorber · 2019-02-09T16:45:09Z

Thanks

I'll open some more PR with docs and examples next week

handle new SwitchRouter history back behaviors

c700bc6

slorber mentioned this pull request Jan 31, 2019

Implement more backBehavior options for SwitchRouter react-navigation/rfcs#2

Merged

brentvatne self-requested a review February 3, 2019 19:08

ericvicenti reviewed Feb 4, 2019

View reviewed changes

ericvicenti suggested changes Feb 4, 2019

View reviewed changes

slorber added 5 commits February 5, 2019 10:08

Merge branch 'master' into switch-back-behavior

5e2b9b2

use toMatchObject

9029cd1

unflatten ifs

8fc2590

update backbehavior implementation according to PR review

4a869c0

use initial route key instead of initial route name

edb3ff0

slorber commented Feb 5, 2019

View reviewed changes

ericvicenti approved these changes Feb 5, 2019

View reviewed changes

brentvatne suggested changes Feb 6, 2019

View reviewed changes

slorber added 2 commits February 6, 2019 11:46

flatten if/else

9fcd977

update getNextState according to PR comments

c753603

slorber commented Feb 6, 2019

View reviewed changes

brentvatne approved these changes Feb 9, 2019

View reviewed changes

brentvatne merged commit 8e0041a into react-navigation:master Feb 9, 2019

		const initialRouteIndex = order.indexOf(initialRouteName);
		if (initialRouteIndex === -1) {

handle new SwitchRouter history back behaviors #31

handle new SwitchRouter history back behaviors #31

Conversation

slorber commented Jan 31, 2019 • edited by brentvatne Loading

slorber commented Jan 31, 2019 • edited Loading

brentvatne commented Feb 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericvicenti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentvatne commented Feb 5, 2019

Choose a reason for hiding this comment

slorber commented Feb 5, 2019

slorber Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericvicenti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentvatne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentvatne commented Feb 9, 2019

brentvatne commented Feb 9, 2019

slorber commented Feb 9, 2019

slorber commented Jan 31, 2019 •

edited by brentvatne

Loading

slorber commented Jan 31, 2019 •

edited

Loading

slorber Feb 5, 2019 •

edited

Loading