Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle new SwitchRouter history back behaviors #31

Merged
merged 8 commits into from Feb 9, 2019

Conversation

Projects
None yet
3 participants
@slorber
Copy link
Member

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

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 brentvatne self-requested a review Feb 3, 2019

@brentvatne

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

thank you! will review this soon

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

const routeIndexHistory = [initialRouteIndex];

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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.

This comment has been minimized.

Copy link
@slorber

slorber Feb 4, 2019

Author Member

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?

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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.

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 5, 2019

Member

agree with @ericvicenti's comments here :)

This comment has been minimized.

Copy link
@slorber

slorber Feb 5, 2019

Author Member

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
Copy link
Contributor

left a comment

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

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

const routeIndexHistory = [initialRouteIndex];

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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.

@@ -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') {

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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

// 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 {

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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.

This comment has been minimized.

Copy link
@slorber

slorber Feb 4, 2019

Author Member

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

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 4, 2019

Contributor

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

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 5, 2019

Member

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.

This comment has been minimized.

Copy link
@slorber

slorber Feb 5, 2019

Author Member

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

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 5, 2019

Contributor

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); )

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 6, 2019

Member

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

This comment has been minimized.

Copy link
@slorber

slorber Feb 6, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

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

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

This comment has been minimized.

Copy link
@slorber

slorber Feb 5, 2019

Author Member

moved this check closer to initialRouteIndex, seems appropriate

@slorber

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

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

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

This comment has been minimized.

Copy link
@slorber

slorber Feb 5, 2019

Author Member

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

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 5, 2019

Contributor

Makes sense that its a shallow match 👌

@ericvicenti
Copy link
Contributor

left a comment

Looking great!

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

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 5, 2019

Contributor

looks like you can flatten this out again 😛

// 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 {

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 5, 2019

Contributor

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); )

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

This comment has been minimized.

Copy link
@ericvicenti

ericvicenti Feb 5, 2019

Contributor

Makes sense that its a shallow match 👌

@brentvatne
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 6, 2019

Member

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

This comment has been minimized.

Copy link
@slorber

slorber Feb 6, 2019

Author Member

done

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

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 6, 2019

Member

same as above re: moving to getNextState

This comment has been minimized.

Copy link
@slorber

slorber Feb 6, 2019

Author Member

done

// 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 {

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 6, 2019

Member

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

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

This comment has been minimized.

Copy link
@brentvatne

brentvatne Feb 6, 2019

Member

else if

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

This comment has been minimized.

Copy link
@slorber

slorber Feb 6, 2019

Author Member

@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

}

return nextState;
return updateNextStateHistory(nextState);

This comment has been minimized.

Copy link
@slorber

slorber Feb 6, 2019

Author Member

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

@brentvatne

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

nice work @slorber :)

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

3 checks passed

ci/circleci: install-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: unit-tests Your tests passed on CircleCI!
Details
@brentvatne

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

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

@slorber

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

Thanks

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.