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

fix(router): fix router.replace() navigation across nested navigators #24457

Merged
merged 18 commits into from
Sep 19, 2023

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Sep 15, 2023

Why

Fix ENG-10061
Fix expo/router#793
Fix expo/router#858
Fix expo/router#832

The linkTo function has two issues.

  • It has trouble navigating to screens in different navigators with identical names
  • it cannot perform a replace navigation while in a nested navigator (it can only replace at the currently level).

How

The original plan was to change linkTo to do navigation by deterministic keys, but React Navigation 7.0 is removing key based navigation, so we'll keep with screen names.

This PR rewrites the linkTo function and simplifies the logic into two paths.

push

linkTo will fire a dispatch action on the root navigator with the desired state.

replace

We compare the current navigation state vs the desired state and find the lowest common navigator that can perform a replace action. This would be the <Stack/> or <Tabs /> navigator that the two states share in common. The common navigator (it could be the root navigator if they have nothing in common!) is the source of the replace event

Test Plan

Manually tested and wrote a new test.

Checklist

@linear
Copy link

linear bot commented Sep 15, 2023

ENG-10061 [expo-router] The action 'REPLACE' with payload {"name":"SCREEN"} was not handled by any navigator.

This issue was automatically imported from GitHub: #24353

Issue accepted by mark


Summary:

  • The user is experiencing an issue with navigating from one nested stack to another nested stack within bottom tabs in a React Native application.
  • The error the user encounters is: The action 'REPLACE' with payload {"name":"SCREEN"} was not handled by any navigator.
  • In recreating the error, the user instructs to run the provided repository, click on the second tab and select "Go to Tab2 / Screen 2", then click on the first tab and select "Go to Tab2 / Screen 1", at which point the error appears.
  • The user notes that they intended for this issue to be placed in the expo-router repository, and is not sure why it has ended up in the main Expo repository.
  • The user is utilizing npm as the package manager for the project.

@marklawlor marklawlor added the Router expo-router label Sep 15, 2023
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 15, 2023
@marklawlor marklawlor changed the title fix(router): improving replace navigation across nested navigators fix(router): replace navigation across nested navigators Sep 15, 2023
@marklawlor
Copy link
Contributor Author

@EvanBacon Can you suggest more test scenarios that I should cover here?

@marklawlor marklawlor changed the title fix(router): replace navigation across nested navigators fix(router): fix router.replace() navigation across nested navigators Sep 15, 2023
@marklawlor marklawlor marked this pull request as ready for review September 15, 2023 08:13
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 15, 2023
@marklawlor marklawlor marked this pull request as draft September 15, 2023 08:29
@marklawlor marklawlor marked this pull request as ready for review September 18, 2023 22:09
Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good and tests are passing. Original implementation was a hack.

@EvanBacon EvanBacon merged commit fe80dcf into main Sep 19, 2023
8 checks passed
@EvanBacon EvanBacon deleted the marklawlor/router/ENG-10061 branch September 19, 2023 21:45
EvanBacon added a commit to expo/router that referenced this pull request Sep 19, 2023
EvanBacon added a commit to expo/router that referenced this pull request Sep 19, 2023
…#884)

* fix(router): fix router.replace() navigation across nested navigators

backport fix from expo/expo#24457

* drop stateOperations

* Delete routing.test.node.ts
@janusqa
Copy link

janusqa commented Sep 20, 2023

Hi @marklawlor do you know when it will be available?

@devoren
Copy link

devoren commented Sep 20, 2023

@janusqa This pr is merged and released in the latest version 2.0.7, but in my application replace() does not replace or clear web history and I can go back :(

@janusqa
Copy link

janusqa commented Sep 20, 2023

@devoren thanks will take it for a spin.

EvanBacon pushed a commit that referenced this pull request Sep 20, 2023
…#24457)

# Why

Fix ENG-10061
Fix expo/router#793
Fix expo/router#858
Fix expo/router#832

The `linkTo` function has two issues.

* It has trouble navigating to screens in different navigators with
identical names
* it cannot perform a replace navigation while in a nested navigator (it
can only replace at the currently level).

# How

The original plan was to change `linkTo` to do navigation by
deterministic keys, but React Navigation 7.0 is removing key based
navigation, so we'll keep with screen names.

This PR rewrites the `linkTo` function and simplifies the logic into two
paths.

**push**

`linkTo` will fire a dispatch action on the root navigator with the
desired state.

**replace**

We compare the current navigation state vs the desired state and find
the lowest common navigator that can perform a replace action. This
would be the `<Stack/>` or `<Tabs />` navigator that the two states
share in common. The common navigator (it could be the root navigator if
they have nothing in common!) is the source of the replace event

# Test Plan

Manually tested and wrote a new test.

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
EvanBacon added a commit that referenced this pull request Sep 20, 2023
# Why

- fix regression in #24457

# How

- clone objects to prevent circular references.

# Test Plan

- Added test that could repro the issue.

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@janusqa
Copy link

janusqa commented Sep 24, 2023

@devoren something is wrong here. I open app land on a page. I click a button which uses replace to move to a screen that is not the initial screen in another adjacent stack. The screen has no back button. Back button was expected that i can click on it to go to initial screen of stack. if i click on the bottom tab to take me to the initial screen in that stack then the initial screen has a back button, although it should not since its the initial screen. Will try to do a repro repo and file a bug issue for it.

Also still getting a large warning "The redirect prop on is deprecated and will be removed. Please use router.redirect instead "
Was that not fixed in 2.0.4 or 2.0.5?

marklawlor added a commit that referenced this pull request Dec 18, 2023
# Why

Fix: #25981

# How

To be honest, I'm not 100% sure why its broken in the current state. It
fires a navigation events on the shared Stack with what seems like the
correct params.

But we also don't need to track the last common Stack/Tab
lastCommonNavigator. Pre-#24457, we used to fire navigation events on
the root navigator, so #24457 tried to not deviate from this behaviour
by only changing the behaviour Stacks&Tabs doing replace actions. #24600
cleaned up the logic of #24457 and unified the logic for all navigation
types, but still only tracked the last common Stack/Tabs. I don't know
why this breaks Drawer, it might be an internal implementation detail,
but we can simply fire the event on the last common navigator - no
matter what type it is.

# Test Plan

Add a smoke test for `<Drawer />`. I don't believe it was being
previously tested.

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
onizam95 pushed a commit to onizam95/expo-av-drm that referenced this pull request Jan 15, 2024
# Why

Fix: expo#25981

# How

To be honest, I'm not 100% sure why its broken in the current state. It
fires a navigation events on the shared Stack with what seems like the
correct params.

But we also don't need to track the last common Stack/Tab
lastCommonNavigator. Pre-expo#24457, we used to fire navigation events on
the root navigator, so expo#24457 tried to not deviate from this behaviour
by only changing the behaviour Stacks&Tabs doing replace actions. expo#24600
cleaned up the logic of expo#24457 and unified the logic for all navigation
types, but still only tracked the last common Stack/Tabs. I don't know
why this breaks Drawer, it might be an internal implementation detail,
but we can simply fire the event on the last common navigator - no
matter what type it is.

# Test Plan

Add a smoke test for `<Drawer />`. I don't believe it was being
previously tested.

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about Router expo-router
Projects
None yet
5 participants