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

[router]: ensure navigation events are fired at correct target #27485

Merged
merged 5 commits into from Mar 26, 2024

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Mar 7, 2024

Why

Expo Router was incorrectly firing events with the incorrect navigator target. This would cause situations where the event was ignored or the state in correct.

The result of this was a sublty incorrect routing state that was

  • causing incorrect back history
  • navigation state was at the incorrect level in the hierarchy.

How

I rewrote getNavigateAction to get the action in a single pass. Previously we were getting the action, and then refining the action based up on the parent navigator.

This approach could have worked, but the structure of the navigation action is, well, weird. This made converting an action into the targeted action error prone.

The new approach is to first identify the target, and then build the correct navigation action.

Test Plan

The fixed heirarachy (and incorrect back behaviour) can be demonstrated by this test. Now the routes are correctly pushing to the <Stack /> inside the (group)

This incorrect back behaviour can be demonstrated by this test. Notice how some routes were previously not added to the history, causing the back behaviour to "skip" a route

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 7, 2024
@marklawlor marklawlor force-pushed the marklawlor/router/fix-navigation-target branch from 1e942f9 to 18f423b Compare March 14, 2024 07:25
@marklawlor marklawlor marked this pull request as ready for review March 14, 2024 12:49
@marklawlor marklawlor marked this pull request as draft March 14, 2024 13:01
@marklawlor marklawlor force-pushed the marklawlor/router/fix-navigation-target branch from b38d557 to b599c91 Compare March 14, 2024 13:21
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 14, 2024
@marklawlor marklawlor marked this pull request as ready for review March 14, 2024 13:45
@expo-bot expo-bot added bot: needs changes ExpoBot found things that don't meet our guidelines and removed bot: passed checks ExpoBot has nothing to complain about labels Mar 18, 2024
@marklawlor marklawlor force-pushed the marklawlor/router/fix-navigation-target branch from 4acfdaa to cfbcd75 Compare March 18, 2024 20:44
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: needs changes ExpoBot found things that don't meet our guidelines labels Mar 18, 2024
@marklawlor marklawlor force-pushed the marklawlor/router/fix-navigation-target branch from cfbcd75 to 3cc03d9 Compare March 20, 2024 22:47
@marklawlor marklawlor merged commit 62b3fbe into main Mar 26, 2024
10 checks passed
@marklawlor marklawlor deleted the marklawlor/router/fix-navigation-target branch March 26, 2024 05:10
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint compatible bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants