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] remove search parameters from screen.getId() #27285

Merged
merged 10 commits into from
Mar 1, 2024

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Feb 26, 2024

Why

Expo Router's navigate and push functions don't always behave as expected and have overlapping functionality. E.g push wouldn't always push, and navigate's behaviour would change based upon this history in unexpected ways.

For example, lets say we are on the initial URL /page

action result visual notes
navigate('/page?a=1) /page?a=1 pushed
navigate('/page?b=1') /page?b=1 pushed
navigate('/page') /page rerendered RN thought this was equal to /page?b=1
goBack /page?a=1 pop /page?b=1 skipped

The reason for this behaviour is that RN doesn't preserve the IDs for every screen in the history. It compares them with the current params. The getID function for /page?b=1 and /page are the same, hence this behaviour.

To change how React Navigation does this, we would need to fork the StackNavigator

How

The simple fix is to not include search parameters in the ID generation. This also has the advantage that is now easier to document the differences between push and navigate.

After this PR, Expo Router will have this behaviour

action result visual notes
navigate('/page?a=1) /page?a=1 rerendered
navigate('/page?b=1') /page?b=1 rerendered
navigate('/page') /page rerendered
goBack /page app closes There is no screen to go back, we only rerender the same screen
action result visual notes
push('/page?a=1) /page?a=1 pushed
push('/page?b=1') /page?b=1 pushed
push('/page') /page pushed
goBack() /page?b=1 popped

Test Plan

I've updated the unit tests and also added a new set to test stack unwinding.

Checklist

@marklawlor marklawlor changed the title (router) remove search parameters from screen getId() (router) remove search parameters from screen.getId() Feb 26, 2024
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Feb 26, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Feb 26, 2024
@marklawlor marklawlor changed the title (router) remove search parameters from screen.getId() [router] remove search parameters from screen.getId() Feb 26, 2024
docs/pages/router/navigating-pages.mdx Outdated Show resolved Hide resolved
docs/pages/router/navigating-pages.mdx Outdated Show resolved Hide resolved
docs/pages/router/navigating-pages.mdx Outdated Show resolved Hide resolved
docs/pages/router/navigating-pages.mdx Outdated Show resolved Hide resolved
act(() => router.push('/c'));
act(() => router.push('/a')); // This will unwind the stack

expect(router.canGoBack()).toBe(true); //
Copy link
Contributor

Choose a reason for hiding this comment

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

Should check the expected history too

@marklawlor marklawlor force-pushed the marklawlor/router/remove-searchparams-from-navigate branch from d419cc8 to 6237433 Compare February 27, 2024 01:34
@marklawlor
Copy link
Contributor Author

Blocked by #27307

I want to ensure we have the push behaviour correct before fixing the navigate behaviour. These PRs also modify the same tests, but #27307 adds more detail

@marklawlor marklawlor mentioned this pull request Feb 28, 2024
3 tasks
marklawlor and others added 10 commits February 29, 2024 09:49
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Co-authored-by: Aman Mittal <amandeepmittal@live.com>
Co-authored-by: Aman Mittal <amandeepmittal@live.com>
Co-authored-by: Aman Mittal <amandeepmittal@live.com>
Co-authored-by: Evan Bacon <bacon@expo.io>
Co-authored-by: Evan Bacon <bacon@expo.io>
Co-authored-by: Evan Bacon <bacon@expo.io>
@marklawlor marklawlor force-pushed the marklawlor/router/remove-searchparams-from-navigate branch from 7e6e8d6 to 1b4b362 Compare February 28, 2024 23:50
@marklawlor marklawlor merged commit 2e20b05 into main Mar 1, 2024
13 checks passed
@marklawlor marklawlor deleted the marklawlor/router/remove-searchparams-from-navigate branch March 1, 2024 00:25
@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

5 participants