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

Fix other code paths for resolveTo from a splat route #11045

Merged
merged 2 commits into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/resolve-to-splat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"react-router": patch
"@remix-run/router": patch
---

Fix bug with `resolveTo` in splat routes

- This is a follow up to [#10983](https://github.com/remix-run/react-router/pull/10983) to handle the few other code paths using `getPathContributingMatches`
- This removes the `UNSAFE_getPathContributingMatches` export from `@remix-run/router` since we no longer need this in the `react-router`/`react-router-dom` layers
4 changes: 2 additions & 2 deletions packages/react-router/lib/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
AbortedDeferredError,
Action as NavigationType,
createMemoryHistory,
UNSAFE_getPathContributingMatches as getPathContributingMatches,
UNSAFE_getResolveToMatches as getResolveToMatches,
UNSAFE_invariant as invariant,
parsePath,
resolveTo,
Expand Down Expand Up @@ -281,7 +281,7 @@ export function Navigate({
// StrictMode they navigate to the same place
let path = resolveTo(
to,
getPathContributingMatches(matches).map((match) => match.pathnameBase),
getResolveToMatches(matches),
locationPathname,
relative === "path"
);
Expand Down
15 changes: 3 additions & 12 deletions packages/react-router/lib/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
IDLE_BLOCKER,
Action as NavigationType,
UNSAFE_convertRouteMatchToUiMatch as convertRouteMatchToUiMatch,
UNSAFE_getPathContributingMatches as getPathContributingMatches,
UNSAFE_getResolveToMatches as getResolveToMatches,
UNSAFE_invariant as invariant,
isRouteErrorResponse,
joinPaths,
Expand Down Expand Up @@ -197,9 +197,7 @@ function useNavigateUnstable(): NavigateFunction {
let { matches } = React.useContext(RouteContext);
let { pathname: locationPathname } = useLocation();

let routePathnamesJson = JSON.stringify(
getPathContributingMatches(matches).map((match) => match.pathnameBase)
);
let routePathnamesJson = JSON.stringify(getResolveToMatches(matches));

let activeRef = React.useRef(false);
useIsomorphicLayoutEffect(() => {
Expand Down Expand Up @@ -311,14 +309,7 @@ export function useResolvedPath(
): Path {
let { matches } = React.useContext(RouteContext);
let { pathname: locationPathname } = useLocation();

// Use the full pathname for the leaf match so we include splat values
// for "." links
let routePathnamesJson = JSON.stringify(
getPathContributingMatches(matches).map((match, idx) =>
idx === matches.length - 1 ? match.pathname : match.pathnameBase
)
);
let routePathnamesJson = JSON.stringify(getResolveToMatches(matches));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change on line 319 was only made in the one code path before. To avoid this same mistake in the future, we're now using a utility getResolveToMatches to share this logic.


return React.useMemo(
() =>
Expand Down
26 changes: 25 additions & 1 deletion packages/router/__tests__/path-resolution-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe("path resolution", () => {
path: "*",
},
],
"/foo",
"/foo/bar",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to #10983, we had a test here asserting the previously buggy behavior. A link to "." from a splat should stay on the current location, not route up to the parent

false
);
});
Expand Down Expand Up @@ -391,6 +391,21 @@ describe("path resolution", () => {
]);
});

it("from an index route", () => {
assertRoutingToChild([
{
path: "bar",
children: [
{
id: "activeRoute",
index: true,
},
{ path: "baz" },
],
},
]);
});

it("from a dynamic param route", () => {
assertRoutingToChild([
{
Expand All @@ -400,6 +415,15 @@ describe("path resolution", () => {
},
]);
});

it("from a splat route", () => {
assertRoutingToChild([
{
id: "activeRoute",
path: "*",
},
]);
});
/* eslint-enable jest/expect-expect */
});

Expand Down
2 changes: 1 addition & 1 deletion packages/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export {
ErrorResponseImpl as UNSAFE_ErrorResponseImpl,
convertRoutesToDataRoutes as UNSAFE_convertRoutesToDataRoutes,
convertRouteMatchToUiMatch as UNSAFE_convertRouteMatchToUiMatch,
getPathContributingMatches as UNSAFE_getPathContributingMatches,
getResolveToMatches as UNSAFE_getResolveToMatches,
} from "./utils";

export {
Expand Down
3 changes: 2 additions & 1 deletion packages/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
convertRouteMatchToUiMatch,
convertRoutesToDataRoutes,
getPathContributingMatches,
getResolveToMatches,
immutableRouteKeys,
isRouteErrorResponse,
joinPaths,
Expand Down Expand Up @@ -3338,7 +3339,7 @@ function normalizeTo(
// Resolve the relative path
let path = resolveTo(
to ? to : ".",
getPathContributingMatches(contextualMatches).map((m) => m.pathnameBase),
getResolveToMatches(contextualMatches),
stripBasename(location.pathname, basename) || location.pathname,
relative === "path"
);
Expand Down
11 changes: 11 additions & 0 deletions packages/router/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,17 @@ export function getPathContributingMatches<
);
}

// Return the array of pathnames for the current route matches - used to
// generate the routePathnames input for resolveTo()
export function getResolveToMatches<
T extends AgnosticRouteMatch = AgnosticRouteMatch
>(matches: T[]) {
// Use the full pathname for the leaf match so we include splat values for "." links
return getPathContributingMatches(matches).map((match, idx) =>
idx === matches.length - 1 ? match.pathname : match.pathnameBase
);
}

/**
* @private
*/
Expand Down
Loading