[router]: include search parameters in the default Screen.getId() #26710
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marklawlor
commented
Jan 26, 2024
| * fall back to `key` based navigation. This is an issue for search parameters where are either | ||
| * part of the key or the id. | ||
| */ | ||
| return id || route.contextKey; |
There was a problem hiding this comment.
We now always return an ID value. This prevents React Navigation from falling back to name & key based matching, where it may match on a correct key, but this screen may have had parameters.
marklawlor
commented
Jan 26, 2024
| expect(testRouter.canGoBack()).toBe(false); | ||
| }); | ||
|
|
||
| it.skip('handles popToTop', async () => { |
There was a problem hiding this comment.
We don't have popToTop on the router?. I'll add it in a new PR
expo-bot
added
bot: passed checks
This was referenced Jan 26, 2024
EvanBacon
approved these changes
Jan 29, 2024
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
marklawlor
force-pushed
the
marklawlor/router/getId-search-params
branch
from
64b2922
to
dc0e369
Compare
github-actions
bot
added
bot: fingerprint compatible
and removed
bot: fingerprint changed
labels
|
Related #26916 |
marklawlor
force-pushed
the
marklawlor/router/getId-search-params
branch
from
b0bee6c
to
51dc0f9
Compare
marklawlor
added a commit
that referenced
this pull request
Feb 12, 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 <!-- 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: kennethstarkrl <kenneth@feednom.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
push/navigate/replacedon't work as expected with search parameters. React Navigation uses both the screen's name & ID to detect if the screen is within the stack. If the search parameters are not included in the ID then React Navigation will treat/test?a=true === /test?b=trueHow
I've rewritten the
createGetIdForRoutewith annotations below. There are two main differences_layoutis the same).Test Plan
I've adjusted the existing
getIdtests. All our testing tests still pass, but this is mostly due to lack of coverage.Checklist
npx expo prebuild& EAS Build (eg: updated a module plugin).