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

useNavigate: Get pathname from useLocation instead of RouteContext #7982

Merged
merged 3 commits into from
Sep 1, 2021

Conversation

chaance
Copy link
Collaborator

@chaance chaance commented Aug 31, 2021

In #7880 it looks like useSearchParams, when called from a higher level route that renders an outlet, potentially directs to the wrong pathname when setSearchParams is called and a user is actually at a nested route.

This is actually a problem with useNavigate, I believe. When the resulting navigate function is called without an explicit pathname, we should stick with the current pathname returned by useLocation instead of the pathname constructed in the RouteContext. This ensures that navigate always directs users to a location with the same pathname they are currently on while only modifying either the hash or search depending on what is passed to navigate. Because setSearchParams abstracts navigate, this should fix #7880 and make useNavigate more stable in general.

@chaance chaance requested a review from mjackson August 31, 2021 19:49
@chaance chaance changed the title useNavigate: Get pathname from useLocation instead of RouteContext useNavigate: Get pathname from useLocation instead of RouteContext Aug 31, 2021
@chaance chaance changed the title useNavigate: Get pathname from useLocation instead of RouteContext useNavigate: Get pathname from useLocation instead of RouteContext Aug 31, 2021
@mjackson
Copy link
Member

This looks great 👍 Can we also get some tests in place around this? I'm thinking we should have a test for navigate() with only a search string, and another one for only a hash. Both should make sure we don't manipulate the pathname in the navigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants