Skip to content

fix bug with nested optional segments #9727

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 14, 2022
Merged

fix bug with nested optional segments #9727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e94438b
fix bug with nested optional segments
brophdawg11 Dec 13, 2022
0ce082e
Add changeset and remove console.log
brophdawg11 Dec 13, 2022
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
5 changes: 5 additions & 0 deletions .changeset/slow-drinks-cheer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Fix issue with deeply nested optional segments
240 changes: 235 additions & 5 deletions packages/react-router/__tests__/path-matching-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,31 +458,97 @@ describe("path matching with optional dynamic segments", () => {
});

test("optional params at the end of the path", () => {
let manualRoutes = [
{
path: "/nested",
},
{
path: "/nested/:one",
},
{
path: "/nested/:one/:two",
},
{
path: "/nested/:one/:two/:three",
},
{
path: "/nested/:one/:two/:three/:four",
},
];
let routes = [
{
path: "/nested/:one?/:two?",
path: "/nested/:one?/:two?/:three?/:four?",
},
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated some of these complex tests to include the equivalent manual route definitions and am asserting against both set of routes to ensure we're consistent with what the user would otherwise do manually.


expect(pickPathsAndParams(manualRoutes, "/nested")).toEqual([
{
path: "/nested",
params: {},
},
]);
expect(pickPathsAndParams(routes, "/nested")).toEqual([
{
path: "/nested/:one?/:two?",
path: "/nested/:one?/:two?/:three?/:four?",
params: {},
},
]);
expect(pickPathsAndParams(manualRoutes, "/nested/foo")).toEqual([
{
path: "/nested/:one",
params: { one: "foo" },
},
]);
expect(pickPathsAndParams(routes, "/nested/foo")).toEqual([
{
path: "/nested/:one?/:two?",
path: "/nested/:one?/:two?/:three?/:four?",
params: { one: "foo" },
},
]);
expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar")).toEqual([
{
path: "/nested/:one/:two",
params: { one: "foo", two: "bar" },
},
]);
expect(pickPathsAndParams(routes, "/nested/foo/bar")).toEqual([
{
path: "/nested/:one?/:two?",
path: "/nested/:one?/:two?/:three?/:four?",
params: { one: "foo", two: "bar" },
},
]);
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz")).toEqual(null);
expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz")).toEqual([
{
path: "/nested/:one/:two/:three",
params: { one: "foo", two: "bar", three: "baz" },
},
]);
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz")).toEqual([
{
path: "/nested/:one?/:two?/:three?/:four?",
params: { one: "foo", two: "bar", three: "baz" },
},
]);
expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz/qux")).toEqual(
[
{
path: "/nested/:one/:two/:three/:four",
params: { one: "foo", two: "bar", three: "baz", four: "qux" },
},
]
);
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz/qux")).toEqual([
{
path: "/nested/:one?/:two?/:three?/:four?",
params: { one: "foo", two: "bar", three: "baz", four: "qux" },
},
]);
expect(
pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz/qux/zod")
).toEqual(null);
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz/qux/zod")).toEqual(
null
);
});

test("intercalated optional params", () => {
Expand Down Expand Up @@ -515,6 +581,170 @@ describe("path matching with optional dynamic segments", () => {
});

test("consecutive optional dynamic segments in nested routes", () => {
let manuallyExploded = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some ambiguity here - i.e., would the user put the value there first or not there first? I chose to assume that since we want to match deeper paths, that we would assume you would put the param there first, followed by the version without the param.

{
path: ":one",
children: [
{
path: ":two",
children: [
{
path: ":three",
},
{
path: "",
},
],
},
{
path: "",
children: [
{
path: ":three",
},
{
path: "",
},
],
},
],
},
{
path: "",
children: [
{
path: ":two",
children: [
{
path: ":three",
},
{
path: "",
},
],
},
{
path: "",
children: [
{
path: ":three",
},
{
path: "",
},
],
},
],
},
];

let optional = [
{
path: ":one?",
children: [
{
path: ":two?",
children: [
{
path: ":three?",
},
],
},
],
},
];

expect(pickPathsAndParams(manuallyExploded, "/uno")).toEqual([
{
path: ":one",
params: { one: "uno" },
},
{
params: { one: "uno" },
},
{
params: { one: "uno" },
},
]);
expect(pickPathsAndParams(optional, "/uno")).toEqual([
{
path: ":one?",
params: { one: "uno" },
},
{
params: { one: "uno" },
path: ":two?",
},
{
params: { one: "uno" },
path: ":three?",
},
]);

expect(pickPathsAndParams(manuallyExploded, "/uno/dos")).toEqual([
{
path: ":one",
params: { one: "uno", two: "dos" },
},
{
params: { one: "uno", two: "dos" },
path: ":two",
},
{
params: { one: "uno", two: "dos" },
},
]);
expect(pickPathsAndParams(optional, "/uno/dos")).toEqual([
{
path: ":one?",
params: { one: "uno", two: "dos" },
},
{
params: { one: "uno", two: "dos" },
path: ":two?",
},
{
params: { one: "uno", two: "dos" },
path: ":three?",
},
]);

expect(pickPathsAndParams(manuallyExploded, "/uno/dos/tres")).toEqual([
{
path: ":one",
params: { one: "uno", two: "dos", three: "tres" },
},
{
params: { one: "uno", two: "dos", three: "tres" },
path: ":two",
},
{
params: { one: "uno", two: "dos", three: "tres" },
path: ":three",
},
]);
expect(pickPathsAndParams(optional, "/uno/dos/tres")).toEqual([
{
path: ":one?",
params: { one: "uno", two: "dos", three: "tres" },
},
{
params: { one: "uno", two: "dos", three: "tres" },
path: ":two?",
},
{
params: { one: "uno", two: "dos", three: "tres" },
path: ":three?",
},
]);

expect(pickPathsAndParams(manuallyExploded, "/uno/dos/tres/nope")).toEqual(
null
);
expect(pickPathsAndParams(optional, "/uno/dos/tres/nope")).toEqual(null);
});

test("consecutive optional static + dynamic segments in nested routes", () => {
let nested = [
{
path: "/one/:two?",
Expand Down
42 changes: 26 additions & 16 deletions packages/router/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,25 +468,35 @@ function explodeOptionalSegments(path: string): string[] {
if (rest.length === 0) {
// Intepret empty string as omitting an optional segment
// `["one", "", "three"]` corresponds to omitting `:two` from `/one/:two?/three` -> `/one/three`
return isOptional ? ["", required] : [required];
return isOptional ? [required, ""] : [required];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prefer the presence of the the param when exploding routes

}

let restExploded = explodeOptionalSegments(rest.join("/"));
return restExploded
.flatMap((subpath) => {
// /one + / + :two/three -> /one/:two/three
let requiredExploded =
subpath === "" ? required : required + "/" + subpath;
// For optional segments, return the exploded path _without_ current segment first (`subpath`)
// and exploded path _with_ current segment later (`subpath`)
// This ensures that exploded paths are emitted in priority order
// `/one/three/:four` will come before `/one/three/:five`
return isOptional ? [subpath, requiredExploded] : [requiredExploded];
})
.map((exploded) => {
// for absolute paths, ensure `/` instead of empty segment
return path.startsWith("/") && exploded === "" ? "/" : exploded;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flatmap was the weasel here since it caused he versions where this path was included to be interspersed with all child combinations when we want them all at the front.

// Assume
[a, b] // => [required, optional]
[c, d] // => [required, optional]
[e, f] // => [required, optional]

// We were getting this:
[a,b,c,d,e,f]

// But we wanted this:
[a,c,e,b,d,f]


let result: string[] = [];

// All child paths with the prefix. Do this for all children before the
// optional version for all children so we get consistent ordering where the
// parent optional aspect is preferred as required. Otherwise, we can get
// child sections interspersed where deeper optional segments are higher than
// parent optional segments, where for example, /:two would explodes _earlier_
// then /:one. By always including the parent as required _for all children_
// first, we avoid this issue
result.push(
...restExploded.map((subpath) =>
subpath === "" ? required : [required, subpath].join("/")
)
);

// Then if this is an optional value, add all child versions without
if (isOptional) {
result.push(...restExploded);
}

// for absolute paths, ensure `/` instead of empty segment
return result.map((exploded) =>
path.startsWith("/") && exploded === "" ? "/" : exploded
);
}

function rankRouteBranches(branches: RouteBranch[]): void {
Expand Down