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] fix router.push inside a group #26678

Merged
merged 8 commits into from
Feb 12, 2024
Merged

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Jan 25, 2024

Why

Fix: #26669
Close: #26597

How

#26597 correctly identified that we need to deep compare the params. I cleaned up the logic and added unit tests which are a minimal reproduction of #26669

Test Plan

The navigation.test.ios.tsx file is getting a bit messy, so I created a new push.test.ios.tsx file. Over time I'm going to separate the rest of the navigation tests into a push/navigate/replace files.

See comments for more information

Blocked by

#26710 - Screens with search parameters are being incorrectly handled
PR pending - Screens are not being hoisted correctly. This causes the push to behave incorrectly as the screen is not within the expected _layout

Checklist

@marklawlor marklawlor changed the title Marklawlor/router/26669 [router] fix router.push inside a group Jan 25, 2024
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 25, 2024
import { act, renderRouter, screen } from '../testing-library';
import { Slot } from '../views/Navigator';

it('stacks should always push a new route', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is a simplified version of #26669. There were two problems

  1. It targeted based upon name, so it always targeted (group)/_layout
  2. Once that was fixed it didn't target correctly based upon different params, e.g user/2 was targeting the <Stack /> inside user/1/_layout instead of (group)/_layout

Because of the incorrect target, Screens were being nested instead of pushed (hence why they switched instead of animated).

This test shows they are always being pushed

@marklawlor marklawlor marked this pull request as draft January 25, 2024 06:36
return keysA.every((key) => {
const valueA = paramsA[key];
const valueB = paramsB[key];
return valueA === valueB || valueA?.toString?.() === valueB?.toString?.();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return valueA === valueB || valueA?.toString?.() === valueB?.toString?.();
return valueA == valueB

If we're comparing numbers and strings, it would be cheaper to use == than to optionally convert to string.

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, assuming tests pass.

@kennethstarkrl
Copy link
Contributor

@marklawlor This fixes routing pushing to slug routes, however, there is still a bug for the same thing but with url query params. ie route/to/slug**?query=test** Looks like url queries aren't included in the params object.

@marklawlor
Copy link
Contributor Author

This is currently blocked by a number of small changes that I need to make. I'll update the description as a post new PRs

@marklawlor marklawlor merged commit 0546bb9 into main Feb 12, 2024
10 checks passed
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
5 participants