Skip to content

Commit e94438b

Browse files
brophdawg11brophdawg11
committed
fix bug with nested optional segments
1 parent 1cef1a5 commit e94438b

File tree

2 files changed

+262
-21
lines changed

2 files changed

+262
-21
lines changed

‎packages/react-router/__tests__/path-matching-test.tsx‎

Lines changed: 235 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -458,31 +458,97 @@ describe("path matching with optional dynamic segments", () => {
458458
});
459459

460460
test("optional params at the end of the path", () => {
461+
let manualRoutes = [
462+
{
463+
path: "/nested",
464+
},
465+
{
466+
path: "/nested/:one",
467+
},
468+
{
469+
path: "/nested/:one/:two",
470+
},
471+
{
472+
path: "/nested/:one/:two/:three",
473+
},
474+
{
475+
path: "/nested/:one/:two/:three/:four",
476+
},
477+
];
461478
let routes = [
462479
{
463-
path: "/nested/:one?/:two?",
480+
path: "/nested/:one?/:two?/:three?/:four?",
464481
},
465482
];
466483

484+
expect(pickPathsAndParams(manualRoutes, "/nested")).toEqual([
485+
{
486+
path: "/nested",
487+
params: {},
488+
},
489+
]);
467490
expect(pickPathsAndParams(routes, "/nested")).toEqual([
468491
{
469-
path: "/nested/:one?/:two?",
492+
path: "/nested/:one?/:two?/:three?/:four?",
470493
params: {},
471494
},
472495
]);
496+
expect(pickPathsAndParams(manualRoutes, "/nested/foo")).toEqual([
497+
{
498+
path: "/nested/:one",
499+
params: { one: "foo" },
500+
},
501+
]);
473502
expect(pickPathsAndParams(routes, "/nested/foo")).toEqual([
474503
{
475-
path: "/nested/:one?/:two?",
504+
path: "/nested/:one?/:two?/:three?/:four?",
476505
params: { one: "foo" },
477506
},
478507
]);
508+
expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar")).toEqual([
509+
{
510+
path: "/nested/:one/:two",
511+
params: { one: "foo", two: "bar" },
512+
},
513+
]);
479514
expect(pickPathsAndParams(routes, "/nested/foo/bar")).toEqual([
480515
{
481-
path: "/nested/:one?/:two?",
516+
path: "/nested/:one?/:two?/:three?/:four?",
482517
params: { one: "foo", two: "bar" },
483518
},
484519
]);
485-
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz")).toEqual(null);
520+
expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz")).toEqual([
521+
{
522+
path: "/nested/:one/:two/:three",
523+
params: { one: "foo", two: "bar", three: "baz" },
524+
},
525+
]);
526+
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz")).toEqual([
527+
{
528+
path: "/nested/:one?/:two?/:three?/:four?",
529+
params: { one: "foo", two: "bar", three: "baz" },
530+
},
531+
]);
532+
expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz/qux")).toEqual(
533+
[
534+
{
535+
path: "/nested/:one/:two/:three/:four",
536+
params: { one: "foo", two: "bar", three: "baz", four: "qux" },
537+
},
538+
]
539+
);
540+
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz/qux")).toEqual([
541+
{
542+
path: "/nested/:one?/:two?/:three?/:four?",
543+
params: { one: "foo", two: "bar", three: "baz", four: "qux" },
544+
},
545+
]);
546+
expect(
547+
pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz/qux/zod")
548+
).toEqual(null);
549+
expect(pickPathsAndParams(routes, "/nested/foo/bar/baz/qux/zod")).toEqual(
550+
null
551+
);
486552
});
487553

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

517583
test("consecutive optional dynamic segments in nested routes", () => {
584+
let manuallyExploded = [
585+
{
586+
path: ":one",
587+
children: [
588+
{
589+
path: ":two",
590+
children: [
591+
{
592+
path: ":three",
593+
},
594+
{
595+
path: "",
596+
},
597+
],
598+
},
599+
{
600+
path: "",
601+
children: [
602+
{
603+
path: ":three",
604+
},
605+
{
606+
path: "",
607+
},
608+
],
609+
},
610+
],
611+
},
612+
{
613+
path: "",
614+
children: [
615+
{
616+
path: ":two",
617+
children: [
618+
{
619+
path: ":three",
620+
},
621+
{
622+
path: "",
623+
},
624+
],
625+
},
626+
{
627+
path: "",
628+
children: [
629+
{
630+
path: ":three",
631+
},
632+
{
633+
path: "",
634+
},
635+
],
636+
},
637+
],
638+
},
639+
];
640+
641+
let optional = [
642+
{
643+
path: ":one?",
644+
children: [
645+
{
646+
path: ":two?",
647+
children: [
648+
{
649+
path: ":three?",
650+
},
651+
],
652+
},
653+
],
654+
},
655+
];
656+
657+
expect(pickPathsAndParams(manuallyExploded, "/uno")).toEqual([
658+
{
659+
path: ":one",
660+
params: { one: "uno" },
661+
},
662+
{
663+
params: { one: "uno" },
664+
},
665+
{
666+
params: { one: "uno" },
667+
},
668+
]);
669+
expect(pickPathsAndParams(optional, "/uno")).toEqual([
670+
{
671+
path: ":one?",
672+
params: { one: "uno" },
673+
},
674+
{
675+
params: { one: "uno" },
676+
path: ":two?",
677+
},
678+
{
679+
params: { one: "uno" },
680+
path: ":three?",
681+
},
682+
]);
683+
684+
expect(pickPathsAndParams(manuallyExploded, "/uno/dos")).toEqual([
685+
{
686+
path: ":one",
687+
params: { one: "uno", two: "dos" },
688+
},
689+
{
690+
params: { one: "uno", two: "dos" },
691+
path: ":two",
692+
},
693+
{
694+
params: { one: "uno", two: "dos" },
695+
},
696+
]);
697+
expect(pickPathsAndParams(optional, "/uno/dos")).toEqual([
698+
{
699+
path: ":one?",
700+
params: { one: "uno", two: "dos" },
701+
},
702+
{
703+
params: { one: "uno", two: "dos" },
704+
path: ":two?",
705+
},
706+
{
707+
params: { one: "uno", two: "dos" },
708+
path: ":three?",
709+
},
710+
]);
711+
712+
expect(pickPathsAndParams(manuallyExploded, "/uno/dos/tres")).toEqual([
713+
{
714+
path: ":one",
715+
params: { one: "uno", two: "dos", three: "tres" },
716+
},
717+
{
718+
params: { one: "uno", two: "dos", three: "tres" },
719+
path: ":two",
720+
},
721+
{
722+
params: { one: "uno", two: "dos", three: "tres" },
723+
path: ":three",
724+
},
725+
]);
726+
expect(pickPathsAndParams(optional, "/uno/dos/tres")).toEqual([
727+
{
728+
path: ":one?",
729+
params: { one: "uno", two: "dos", three: "tres" },
730+
},
731+
{
732+
params: { one: "uno", two: "dos", three: "tres" },
733+
path: ":two?",
734+
},
735+
{
736+
params: { one: "uno", two: "dos", three: "tres" },
737+
path: ":three?",
738+
},
739+
]);
740+
741+
expect(pickPathsAndParams(manuallyExploded, "/uno/dos/tres/nope")).toEqual(
742+
null
743+
);
744+
expect(pickPathsAndParams(optional, "/uno/dos/tres/nope")).toEqual(null);
745+
});
746+
747+
test("consecutive optional static + dynamic segments in nested routes", () => {
518748
let nested = [
519749
{
520750
path: "/one/:two?",

‎packages/router/utils.ts‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,7 @@ export function matchRoutes<
329329

330330
let branches = flattenRoutes(routes);
331331
rankRouteBranches(branches);
332+
console.log(branches.map((b) => ({ path: b.path, score: b.score })));
332333

333334
let matches = null;
334335
for (let i = 0; matches == null && i < branches.length; ++i) {
@@ -468,25 +469,35 @@ function explodeOptionalSegments(path: string): string[] {
468469
if (rest.length === 0) {
469470
// Intepret empty string as omitting an optional segment
470471
// `["one", "", "three"]` corresponds to omitting `:two` from `/one/:two?/three` -> `/one/three`
471-
return isOptional ? ["", required] : [required];
472+
return isOptional ? [required, ""] : [required];
472473
}
473474

474475
let restExploded = explodeOptionalSegments(rest.join("/"));
475-
return restExploded
476-
.flatMap((subpath) => {
477-
// /one + / + :two/three -> /one/:two/three
478-
let requiredExploded =
479-
subpath === "" ? required : required + "/" + subpath;
480-
// For optional segments, return the exploded path _without_ current segment first (`subpath`)
481-
// and exploded path _with_ current segment later (`subpath`)
482-
// This ensures that exploded paths are emitted in priority order
483-
// `/one/three/:four` will come before `/one/three/:five`
484-
return isOptional ? [subpath, requiredExploded] : [requiredExploded];
485-
})
486-
.map((exploded) => {
487-
// for absolute paths, ensure `/` instead of empty segment
488-
return path.startsWith("/") && exploded === "" ? "/" : exploded;
489-
});
476+
477+
let result: string[] = [];
478+
479+
// All child paths with the prefix. Do this for all children before the
480+
// optional version for all children so we get consistent ordering where the
481+
// parent optional aspect is preferred as required. Otherwise, we can get
482+
// child sections interspersed where deeper optional segments are higher than
483+
// parent optional segments, where for example, /:two would explodes _earlier_
484+
// then /:one. By always including the parent as required _for all children_
485+
// first, we avoid this issue
486+
result.push(
487+
...restExploded.map((subpath) =>
488+
subpath === "" ? required : [required, subpath].join("/")
489+
)
490+
);
491+
492+
// Then if this is an optional value, add all child versions without
493+
if (isOptional) {
494+
result.push(...restExploded);
495+
}
496+
497+
// for absolute paths, ensure `/` instead of empty segment
498+
return result.map((exploded) =>
499+
path.startsWith("/") && exploded === "" ? "/" : exploded
500+
);
490501
}
491502

492503
function rankRouteBranches(branches: RouteBranch[]): void {

0 commit comments

Comments
 (0)