Don’t propagate RESET event from switch router to nested stack routers

[serhiipalash]

software	version
react-navigation	3.3.2
react-navigation/core	3.1.0
react-native	https://github.com/expo/react-native/archive/sdk-32.0.0.tar.gz

This pull request fixes the error "There is no route defined for key..." when you supply RESET event to your root StackRouter and it is passed down to one of your nested StackRouters.

Bug:
video: https://drive.google.com/file/d/1QJvZ_C47xLAfXpb6YJfwFQsT2XRSf5Mv/view
snack: https://snack.expo.io/@serhiipalash/test-react-navigation-reset
repo: https://github.com/serhiipalash/test-react-navigation-reset
screenshot:

There is no sense to spread root StackRouter RESET event to all nested StackRouters. You can do like this only if all your StackRouters are isomorphic to each other (have the same routes and configs). In that case potentially your can raise event “Reset to HomeScreen” and your root StackRouter and all your nested StackRouters will fall back to their HomeScreen as they all have it. But if your nested stack routers are not isomorphic to your root stack router (more realistic case), there will be an error “There is no route defined for key HomeScreen” caused by some of them.

This pull request adds “type” property to StackRouter and SwitchRouter and prevents passing down RESET event from SwitchRouter to its nested StackRouters.

To test this fix

git clone https://github.com/serhiipalash/test-react-navigation-reset
cd test-react-navigation-reset
git checkout -b fix-reset-event-propagation
npm install
expo start -c

video: https://drive.google.com/file/d/1QtHU4SqC0KlTA_9Bn_SZyu08ol7JHaqD/view

P.S. I am not sure why, but the bug happens only when you dispatch RESET event from SwitchRouter screen. When you do the same on one of root StackRouter screens, everything works fine.

https://drive.google.com/file/d/11PcjeIVmEieVJ-Ccw03rKae0hRiIst_x/view

P.P.S @ericvicenti can you please review this pr? If you remember, you reviewed one of my pr for fixing navigation reseting react-navigation/react-navigation#3593.

[serhiipalash]

P.S. I am not sure why, but the bug happens only when you dispatch RESET event from SwitchRouter screen. When you do the same on one of root StackRouter screens, everything works fine.

I've tested everything again and the bug could be fix by changing

function isResetToRootStack(action) {
  return action.type === StackActions.RESET && action.key === null;
}

to

function isResetToRootStack(action) {
  return action.type === StackActions.RESET && action.key == null;
}

in StackRouter.js

@ericvicenti, @brentvatne should I update this pr?

[serhiipalash]

should I update this pr?

It's just I am not sure what is the expected behaviour for reseting navigation. Maybe you want user to be able to reset some part of app navigation without updating anything else (partial reseting)? Then we should only fix isResetToRootStack function, else we should prevent RESET event propagation in nested routers.

[ericvicenti]

So, this may be a documentation/types problem, but it looks like your reset action is lacking a key.

NavigationActions.reset({
  key: 'myStack', // key of the parent navigation state (if there is a parent navigator, route.key)
  index: 1,
  routes: [{...}, {...}],
});

Maybe you want user to be able to reset some part of app navigation without updating anything else (partial reseting)?

Exactly right. The key is provided to target which stack needs to be reset. By specifying the key, the behavior will not get passed down to children routers.

[ericvicenti]

So I don't think this change is necessary. But we should make it clear that key is required with the reset action, maybe by throwing an error.

[brentvatne]

fwiw "reset" is a bit awkward and i'd really love for this to be implemented: react-navigation/rfcs#70

[serhiipalash]

So I don't think this change is necessary. But we should make it clear that key is required with the reset action, maybe by throwing an error.

@ericvicenti Ok, I understand. This seems like a breaking change, as I don't remember that "key" was required before, and without the "key" navigation can crash depending on the screen where you try to use RESET.

Maybe we can just work with undefined as null in "key" prop and make the docs synced with the code in this way?

function isResetToRootStack(action) {
  return action.type === StackActions.RESET && action.key == null;
}

Another question, why does RESET action can crash the app in any way? Why if I have screens "Home", "Settings" and try to reset to "Profile", app will crash with error "There is no route defined for key Profile"? In opposite, when I have screens "Home" and "Settings" and try to navigate to "Profile", nothing happens. This should be an intended behaviour of any action - if it can't be processed, it should do nothing, IMHO. I can create a pr for this, if you want to.

[brentvatne]

@serhiipalash - i think the best option here is to change the StackActions.reset creator so that if key is not provided then key defaults to null. https://github.com/react-navigation/react-navigation-core/blob/master/src/routers/StackActions.js#L23-L26

[serhiipalash]

i think the best option here is to change the StackActions.reset creator so that if key is not provided then key defaults to null.

I made a new pr #44

I tested its changes in https://github.com/serhiipalash/test-react-navigation-reset and everything works fine both when key is missing (root stack is reseted) and when it is set to nested navigator key (Profile stack is reseted).