Fix: `useResolvedPath` and `useNavigate` use different logic #8985

david-crespo · 2022-06-14T21:47:02Z

Discussed in Discord (thread). #8697 and #8954 updated the logic in useNavigate without making the corresponding change to useResolvedPath, which is used to set the href on Links. This means there are cases where the href is correct but the navigate behavior is wrong and vice versa.

Going to try to add a test that reproduces the bug I was seeing.

changeset-bot · 2022-06-14T21:47:04Z

🦋 Changeset detected

Latest commit: e70cf75

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
react-router	Patch
react-router-dom	Patch
react-router-dom-v5-compat	Patch
react-router-native	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

remix-cla-bot · 2022-06-14T21:47:05Z

Hi @david-crespo,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

remix-cla-bot · 2022-06-14T21:54:21Z

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

david-crespo · 2022-06-14T22:05:02Z

I see the failing tests, taking a look. Seems like maybe the pathContributingRoutes logic is not quite right.

david-crespo · 2022-06-14T22:15:48Z

I noticed that in this test (which this PR causes to fail), the description does not match what is asserted. This PR makes it the root URL and not /about.

react-router/packages/react-router-dom/__tests__/link-href-test.tsx

Lines 54 to 70 in 546d778

    
             test('<Link to=".."> with more .. segments than parent routes resolves to the root URL', () => { 
        
               let renderer: TestRenderer.ReactTestRenderer; 
        
               TestRenderer.act(() => { 
        
                 renderer = TestRenderer.create( 
        
                   <MemoryRouter initialEntries={["/inbox/messages"]}> 
        
                     <Routes> 
        
                       <Route path="inbox"> 
        
                         <Route path="messages" element={<Link to="../../about" />} /> 
        
                       </Route> 
        
                     </Routes> 
        
                   </MemoryRouter> 
        
                 ); 
        
               }); 
        
               expect(renderer.root.findByType("a").props.href).toEqual("/about"); 
        
             }); 
        
           });

brophdawg11 · 2022-06-15T14:02:20Z

packages/react-router-dom/__tests__/link-href-test.tsx

@@ -58,7 +58,7 @@ describe("<Link> href", () => {
          <MemoryRouter initialEntries={["/inbox/messages"]}>
            <Routes>
              <Route path="inbox">
-                <Route path="messages" element={<Link to="../../about" />} />
+                <Route path="messages" element={<Link to="../../../about" />} />


This test was incorrect before - ../../about would be the correct # of ..'s. The test passed but it wasn't asserting having more segments than parent routes.

isn't this link going completely out of the routes? .. traverses the route hierarchy, so:

the route hierarchy at /inbox/messages is [inbox, messages]

.. goes to [inbox]

../.. goes to []

../../.. goes completely out of everything

can you help me understand?

From the description, I thought that was the point of this test — showing that even if you accidentally put too many .. you’ll still get / instead of an error or something nonsensical.

yeah that was my understanding as well from the test description, proving that links are "safe" insofar that you can't goof yourself by doing too many

Looking at this test and the one above in the original commit they were added, it seems clear this is just about whether there are more .. than parent Route components. The first test shows that .. takes you to the parent Route, and the second shows that you can resolve ../../ to / even when there is no explicit <Route path="/">. So it does seem like ../../about is enough to satisfy the goal of the test. I changed them back.

It could be worth changing more .. segments than parent routes to more .. segments than parent <Route>s or something to avoid this confusion.

react-router/packages/react-router-dom/__tests__/link-href-test.tsx

Lines 31 to 58 in 15cdd0d

test('<Link to=".."> resolves relative to the parent route', () => {

let renderer = createTestRenderer(

<MemoryRouter initialEntries={["/inbox/messages"]}>

<Routes>

<Route path="inbox">

<Route path="messages" element={<Link to=".." />} />

</Route>

</Routes>

</MemoryRouter>

);

expect(renderer.root.findByType("a").props.href).toEqual("/inbox");

});

test('<Link to=".."> with more .. segments than parent routes resolves to the root URL', () => {

let renderer = createTestRenderer(

<MemoryRouter initialEntries={["/inbox/messages"]}>

<Routes>

<Route path="inbox">

<Route path="messages" element={<Link to="../../about" />} />

</Route>

</Routes>

</MemoryRouter>

);

expect(renderer.root.findByType("a").props.href).toEqual("/about");

});

});

I'm fine if we change them back 👍 . Michael's on vacation this week so we won't be able to get any clarity on exactly what he may have envisioned as the purpose for the tests so probably best to leave them unchanged. My main concern was that we weren't properly asserting the following line in resolveTo:

// If there are more ".." segments than parent routes, resolve relative to // the root / URL. from = routePathnameIndex >= 0 ? routePathnames[routePathnameIndex] : "/";

But - funny enough, even if we change that to from = routePathnames[routePathnameIndex] everything still works since from being undefined causes it to default to "/" in resolvePath anyway so we still get the same behavior.

As to the original commit intent - I think it makes sense that ../../about still passes - I would argue that it's not asserting the "more than" aspect of the description. ../../about should go back to /about because its correctly traversing the exact number of parents that it has. Adding the third .. tests the scenario that you use "more than". I altered the test locally to show how I'm thinking about it:

test('<Link to=".."> with more .. segments than parent routes resolves to the root URL', () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( <MemoryRouter initialEntries={["/inbox/messages"]}> <Routes> <Route path="inbox"> <Route path="messages" element={ <> <Link to="./about" /> // 1 <Link to="../about" /> // 2 <Link to="../../about" /> // 3 <Link to="../../../about" /> // 4 </> } /> </Route> </Routes> </MemoryRouter> ); }); expect(renderer.root.findAllByType("a").map((a) => a.props.href)).toEqual([ "/inbox/messages/about", "/inbox/about", "/about", "/about", ]); });

Link 2 and 3 assert the same thing - that we can traverse upwards with ..

Link 4 asserts something different - specifically when we have "more than"

I do like the clarity of having all 4!

Ah sorry, I reviewed too quickly, the test says "more than parents" so I like this change.

K, I’ll revert my revert.

Spoke to Ryan - going to merge this as-is - these existing tests are a bit tangentially related so I'll re-make the changes in another PR 👍

brophdawg11 · 2022-06-15T14:02:34Z

packages/react-router-dom/__tests__/link-href-test.tsx

@@ -132,7 +132,7 @@ describe("<Link> href", () => {
              <Route path="inbox">
                <Route
                  path="messages/:id"
-                  element={<Link to="../../about" />}
+                  element={<Link to="../../../about" />}


same as above

brophdawg11 · 2022-06-15T14:05:55Z

packages/react-router-dom/__tests__/link-href-test.tsx

@@ -193,7 +193,7 @@ describe("<Link> href", () => {
        );
      });

-      expect(renderer.root.findByType("a").props.href).toEqual("/inbox");
+      expect(renderer.root.findByType("a").props.href).toEqual("/");


This .. should ignore the index route and thus be relative to inbox which would resolve to the root. The incorrect nature of this test can be seen by comparing to the test above where in this same layout, <Link to="."> resolves to /inbox - so .. should resolve to /

brophdawg11 · 2022-06-15T14:06:14Z

packages/react-router-dom/__tests__/link-href-test.tsx

@@ -278,7 +278,7 @@ describe("<Link> href", () => {
        );
      });

-      expect(renderer.root.findByType("a").props.href).toEqual("/inbox");
+      expect(renderer.root.findByType("a").props.href).toEqual("/");


Same as above

brophdawg11 · 2022-06-15T14:08:23Z

Thanks for the PR @david-crespo! I agree some of those existing tests were incorrect given the bug fix this is addressing 👍 . Going to kick this over to @ryanflorence for a final approval but this should be good to go.

david-crespo · 2022-06-15T14:42:12Z

packages/react-router/lib/hooks.tsx

  let routePathnamesJson = JSON.stringify(
-    pathContributingMatches.map((match) => match.pathnameBase)
+    getPathContributingMatches(matches).map((match) => match.pathnameBase)


I had a feeling doing the JSON.stringify inline in both spots was where it would end up.

yeah it certainly worked fine, just felt a little distanced in the shared util since it's a dependency implementation detail for the individual hooks

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

share matches logic between useResolvedPath and useNavigate

6bc6fa7

sign CLA

282454c

remix-cla-bot bot added the CLA Signed label Jun 14, 2022

david-crespo changed the title ~~Share logic between useResolvedPath and useNavigate~~ Fix: useResolvedPath and useNavigate use different logic Jun 14, 2022

brophdawg11 self-assigned this Jun 14, 2022

david-crespo mentioned this pull request Jun 14, 2022

Add icons, user menu to topbar oxidecomputer/console#889

Merged

brophdawg11 added 2 commits June 15, 2022 09:51

Fix 2 tests and add some comments

6707295

add changeset

7f4f210

brophdawg11 reviewed Jun 15, 2022

View reviewed changes

update test descriptions for clarity

d3e215c

brophdawg11 reviewed Jun 15, 2022

View reviewed changes

brophdawg11 marked this pull request as ready for review June 15, 2022 14:07

david-crespo commented Jun 15, 2022

View reviewed changes

brophdawg11 linked an issue Jun 16, 2022 that may be closed by this pull request

[Bug]: Relative navigation issue while navigating from "index" route: ".." doesn't works, I must use "../.." to go back #8350

Closed

brophdawg11 mentioned this pull request Jun 16, 2022

[Bug]: Relative navigation issue while navigating from "index" route: ".." doesn't works, I must use "../.." to go back #8350

Closed

david-crespo mentioned this pull request Jun 20, 2022

Roles page for orgs oxidecomputer/console#948

Merged

change back the ../../.. to ../..

e70cf75

brophdawg11 merged commit e6b6811 into remix-run:dev Jun 21, 2022

brophdawg11 mentioned this pull request Jun 22, 2022

Fix broken tests from conflicting PRs #9004

Merged

github-actions bot mentioned this pull request Jun 22, 2022

chore: Update version for release (pre) #9006

Merged

david-crespo mentioned this pull request Jun 22, 2022

React Router 6.4-pre.8 oxidecomputer/console#936

Merged

brophdawg11 mentioned this pull request Jun 27, 2022

ci: strengthen relative navigation via .. tests #9018

Merged

github-actions bot mentioned this pull request Sep 13, 2022

chore: Update version for release #9239

Merged

brophdawg11 pushed a commit that referenced this pull request Mar 27, 2024

chore: Update version for release (pre) (#8985)

efef744

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: `useResolvedPath` and `useNavigate` use different logic #8985

Fix: `useResolvedPath` and `useNavigate` use different logic #8985

david-crespo commented Jun 14, 2022 •

edited

Loading

changeset-bot bot commented Jun 14, 2022 •

edited

Loading

remix-cla-bot bot commented Jun 14, 2022

remix-cla-bot bot commented Jun 14, 2022

david-crespo commented Jun 14, 2022 •

edited

Loading

david-crespo commented Jun 14, 2022

brophdawg11 Jun 15, 2022

ryanflorence Jun 20, 2022 •

edited

Loading

david-crespo Jun 21, 2022 •

edited

Loading

brophdawg11 Jun 21, 2022

david-crespo Jun 21, 2022 •

edited

Loading

brophdawg11 Jun 21, 2022

david-crespo Jun 21, 2022

ryanflorence Jun 21, 2022

david-crespo Jun 21, 2022

brophdawg11 Jun 21, 2022

brophdawg11 Jun 15, 2022 •

edited

Loading

brophdawg11 Jun 15, 2022

brophdawg11 Jun 15, 2022 •

edited

Loading

brophdawg11 commented Jun 15, 2022

david-crespo Jun 15, 2022

brophdawg11 Jun 15, 2022

	test('<Link to=".."> resolves relative to the parent route', () => {
	let renderer = createTestRenderer(
	<MemoryRouter initialEntries={["/inbox/messages"]}>
	<Routes>
	<Route path="inbox">
	<Route path="messages" element={<Link to=".." />} />
	</Route>
	</Routes>
	</MemoryRouter>
	);

	expect(renderer.root.findByType("a").props.href).toEqual("/inbox");
	});

	test('<Link to=".."> with more .. segments than parent routes resolves to the root URL', () => {
	let renderer = createTestRenderer(
	<MemoryRouter initialEntries={["/inbox/messages"]}>
	<Routes>
	<Route path="inbox">
	<Route path="messages" element={<Link to="../../about" />} />
	</Route>
	</Routes>
	</MemoryRouter>
	);

	expect(renderer.root.findByType("a").props.href).toEqual("/about");
	});
	});

Fix: useResolvedPath and useNavigate use different logic #8985

Fix: useResolvedPath and useNavigate use different logic #8985

Conversation

david-crespo commented Jun 14, 2022 • edited Loading

changeset-bot bot commented Jun 14, 2022 • edited Loading

🦋 Changeset detected

remix-cla-bot bot commented Jun 14, 2022

remix-cla-bot bot commented Jun 14, 2022

david-crespo commented Jun 14, 2022 • edited Loading

david-crespo commented Jun 14, 2022

Choose a reason for hiding this comment

ryanflorence Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

david-crespo Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brophdawg11 Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brophdawg11 Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

brophdawg11 commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix: `useResolvedPath` and `useNavigate` use different logic #8985

Fix: `useResolvedPath` and `useNavigate` use different logic #8985

david-crespo commented Jun 14, 2022 •

edited

Loading

changeset-bot bot commented Jun 14, 2022 •

edited

Loading

david-crespo commented Jun 14, 2022 •

edited

Loading

ryanflorence Jun 20, 2022 •

edited

Loading

david-crespo Jun 21, 2022 •

edited

Loading

david-crespo Jun 21, 2022 •

edited

Loading

brophdawg11 Jun 15, 2022 •

edited

Loading

brophdawg11 Jun 15, 2022 •

edited

Loading