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

feat(router): BREAKING - router.push now uses PUSH navigation #24600

Merged
merged 24 commits into from
Dec 15, 2023

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Sep 25, 2023

Why

router.push() should always most history forwards. However we are currently using the NAVIGATE action which may move history backwards if the screen is in history and the closest navigator is a Stack.

Adds router.navigate for when users wish to opt out of this behaviour.

Fix: expo/router#758
Fix: expo/router#741

Keeping this in draft until #24598 is merged

How

This changes router.push() to a PUSH action if the lowest nested navigator is a stack and shared between the old & new routes. Otherwise it defaults back to NAVIGATE.

Test Plan

Included test case

Checklist

@marklawlor marklawlor added the Router expo-router label Sep 25, 2023
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 25, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 25, 2023
@marklawlor
Copy link
Contributor Author

Change name to pushOrPop

@marklawlor marklawlor force-pushed the marklawlor/router/router-navigate branch from 5c6fd91 to db6cd02 Compare September 28, 2023 22:39
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Sep 28, 2023
@marklawlor marklawlor changed the title feat(router): add router.navigate, change router.push to attempt PUSH navigation feat(router): BREAKING - add router.pushOrPop, change router.push to attempt PUSH navigation Sep 28, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 28, 2023
@marklawlor
Copy link
Contributor Author

Add navigation event types (replace/push/pushOrPop) to <Link />

@hasanaktas
Copy link

Is there an estimated time for release?

@marklawlor
Copy link
Contributor Author

Most likely v3, as this is a breaking change.

@marklawlor marklawlor self-assigned this Nov 27, 2023
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.

Update the docs too and add a guide about differences between push / pushOrPop.

@marklawlor marklawlor force-pushed the marklawlor/router/router-navigate branch from a8a4c68 to 8732d6e Compare December 12, 2023 07:18
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Dec 12, 2023
@marklawlor marklawlor changed the title feat(router): BREAKING - add router.pushOrPop, change router.push to attempt PUSH navigation feat(router): BREAKING - router.push now uses PUSH navigation Dec 13, 2023
@marklawlor marklawlor force-pushed the marklawlor/router/router-navigate branch from d430a9f to 0a2c085 Compare December 14, 2023 06:22
@EvanBacon EvanBacon merged commit 05a353f into main Dec 15, 2023
13 checks passed
@EvanBacon EvanBacon deleted the marklawlor/router/router-navigate branch December 15, 2023 03:21
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Dec 15, 2023
@kennethstarkrl
Copy link
Contributor

@marklawlor The push issue still exists for me after this change in v3.
Doing a router.push() to the same route with different params changes the url with the new params, but the page doesn't change.

I commented out the following from /expo-router/build/global-state/routing.js getNavigateAction() and now push is working and navigates as I expected to.

if (route.state && routesAreEqual && currentRoute.state) {
    return getNavigateAction(route.state, currentRoute.state, type);
}

My guess is that since only the route params changed, all the routes and states are equal. Might need an additional check to see if the params are equal as well.

I modified it adding a params are equal check and it seems to be working
(not entirely sure if this is optimal or correct)

function getNavigateAction(state, parentState, type = 'NAVIGATE') {

    const route = state.routes[state.routes.length - 1];

    const currentRoute = parentState.routes.find((parentRoute) => parentRoute.name === route.name);
    const routesAreEqual = parentState.routes[parentState.index] === currentRoute;
    const paramsAreEqual = rewriteNavigationStateToParams(parentState).params === rewriteNavigationStateToParams(state).params;
    // If there is nested state and the routes are equal, we should keep going down the tree
    if (route.state && routesAreEqual && currentRoute.state && paramsAreEqual) {
        return getNavigateAction(route.state, currentRoute.state, type);
    }
    // Either we reached the bottom of the state or the point where the routes diverged
    const { screen, params } = rewriteNavigationStateToParams(state);
    if (type === 'PUSH' && parentState.type !== 'stack') {
        type = 'NAVIGATE';
    }
    else if (type === 'REPLACE' && parentState.type === 'tab') {
        type = 'JUMP_TO';
    }
    return {
        type,
        target: parentState.key,
        payload: {
            name: screen,
            params,
        },
    };
}

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
…4600)

# Why

`router.push()` should always most history forwards. However we are
currently using the `NAVIGATE` action which may move history backwards
if the screen is in history and the closest navigator is a Stack.

Adds `router.navigate` for when users wish to opt out of this behaviour.

Fix: expo/router#758
Fix: expo/router#741

Keeping this in draft until expo#24598 is
merged

# How

This changes `router.push()` to a PUSH action if the lowest nested
navigator is a stack and shared between the old & new routes. Otherwise
it defaults back to NAVIGATE.

# Test Plan

Included test case

# Checklist

- [ ] 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>
Co-authored-by: Evan Bacon <bacon@expo.io>
Co-authored-by: Aman Mittal <amandeepmittal@live.com>
Co-authored-by: Bartosz Kaszubowski <gosimek@gmail.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: fingerprint compatible bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm Router expo-router
Projects
None yet
8 participants