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

[Bug]: Regression of useResolvedPath in 6.19.0 #11048

Closed
sgoll opened this issue Nov 21, 2023 · 6 comments
Closed

[Bug]: Regression of useResolvedPath in 6.19.0 #11048

sgoll opened this issue Nov 21, 2023 · 6 comments
Labels

Comments

@sgoll
Copy link
Contributor

sgoll commented Nov 21, 2023

What version of React Router are you using?

6.19.0

Steps to Reproduce

Use useResolvedPath() with relative path inside double-nested <Route/>.

See https://github.com/sgoll/react-router-test for a full reproduction with more details.

Expected Behavior

useResolvedPath() should return the same path as in 6.18.0 and be independent of nested routes.

Actual Behavior

When nested in routes within routes (double-nesting), useResolvedPath() returns one level more than expected; this is different from before (6.18.0) and different from the top-level behaviour.

The behaviour is also not consistent when adjusting to explicit relative paths like ./dolor or ../dolor. The first doesn't seem to change anything, and the second strips away two levels simultaneously.

Screenshots

This seems okay (6.18.0) This seems wrong (6.19.0)
react-router-6 18 0 react-router-6 19 0
useResolvedPath("dolor") -> {"pathname":"/ipsum/dolor",…} useResolvedPath("dolor") -> {"pathname":"/ipsum/dolor/dolor",…}
@sgoll sgoll added the bug label Nov 21, 2023
@sgoll sgoll changed the title [Bug]: Regression of useResolvedPath in 6.19 [Bug]: Regression of useResolvedPath in 6.19.0 Nov 21, 2023
@sgoll
Copy link
Contributor Author

sgoll commented Nov 21, 2023

I think this might be related to the bugfix in #10983.

@brophdawg11 @mjackson I am not sure if the behaviour that I am seeing above is now actually intended behaviour or an unintended side effect of the bugfix.

@brophdawg11
Copy link
Contributor

This is the intended behavior - what you're showing here was the previous buggy behavior that in splat routes, useResolvedPath was relative to the current route location without without the splat. There's a bit of a longer explanation here but the general idea in your reproduction is that when you do useResolvedPath("dolor") from the element rendered at ipsum/* while the URL is /ipsum/dolor, it will be relative to /ipsum/dolor and thus will append and become /ipsum/dolor/dolor

@brophdawg11 brophdawg11 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2023
@sgoll
Copy link
Contributor Author

sgoll commented Nov 22, 2023

@brophdawg11 Thank you for the explanation 👍

How would I be able to get the original result? Your link suggests that I could use useResolvedPath("../dolor") but that resolves to /dolor instead of /ipsum/dolor. In one case the splat is part of the location ("dolor" and "./dolor" both resolve to /ipsum/dolor/dolor), in the other it isn't ("../dolor" resolves to /dolor). Is this intended behavior?

To clarify, my use case is an "active route" detection very similar to what <NavLink/> does internally.

This is basically what I have:

function Container() {
  return (
    <>
      <CustomLink path="dolor" />
      <CustomLink path="amet" />
    </>
  );
}

function Dolor(props: { path: string }) {
  const navigate = useNavigate();

  const path = useResolvedPath(props.path);
  const match = useMatch(path.pathname);
  const active = match !== null;

  const handleClick = () => {
    navigate(props.path);
  };

  return (
    <span data-active={active} onClick={handleClick}>{}</span>
  );
}

Previously, both instances of props.path would behave symmetrically. When rendered on route /lorem/*,

  • useResolvedPath("dolor") would yield /lorem/dolor and
  • navigate("dolor") would redirect to /lorem/dolor.

With the recent change to useResolvedPath() this symmetry seems broken.

@chinfeng
Copy link

chinfeng commented Nov 23, 2023

I have the same problem. Now I am unable to deal with navigate inside '/path/to/*' route ...

eg:

  • previous:
const to = useResolvedPath('path/to');
const handle = () => navigate(to); // it will navigate to /base/path/to in any situation.
  • now
const to = useResolvedPath('path/to');
const handle = () => navigate(to); // it will navigate /base/path/to/path/to/path/to/... depends on my current path.

@brophdawg11
Copy link
Contributor

Your link suggests that I could use useResolvedPath("../dolor") but that resolves to /dolor instead of /ipsum/dolor

This is based on how you've set up the route hierarchy. In the linked example, the splat is a level all by itself, which makes it easier to drop just the splat portion by going up one level. You have a static portion and a splat potion in your setup which means .. drops both of them.

So you could change from <Route path="lorem/*" /> to <Route path="lorem"><Route path="*" /></Route> and then you can use .. accordingly.

Otherwise I would leverage useLocation/useParams to replace the splat portion of the path with a new splat value: https://codesandbox.io/p/sandbox/gifted-sound-2683p2

this symmetry seems broken

Try this on 6.20.0 - we found a few additional codepaths that needed the same update we made for useResolvedPath in #10983 and released those in 6.20.0 (via #11045):
https://github.com/remix-run/react-router/blob/main/CHANGELOG.md#v6200

@fzaninotto
Copy link

Please reopen this issue.

I understand that the previous behavior was buggy, but if fixing that bug breaks existing apps, that's a breaking change, and it shouldn't be published in a minor release. According to semver, the fix for useResolvedPath that landed in 6.19 should have landed in a major version.

We built react-admin, which powers at least 20,000 live applications, on top of the previous version. Our code included a fix for the useResolvedPath problem. Your "fix" that landed in 6.19 breaks all these apps (cf #11053). Worse: it's impossible to publish a fix in react-admin that works in all versions of react-router.

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

No branches or pull requests

4 participants