fix bug with nested optional segments #9727
Conversation
🦋 Changeset detectedLatest commit: 0ce082e The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
| let routes = [ | ||
| { | ||
| path: "/nested/:one?/:two?", | ||
| path: "/nested/:one?/:two?/:three?/:four?", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
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.
| @@ -515,6 +581,170 @@ describe("path matching with optional dynamic segments", () => { | |||
| }); | |||
|
|
|||
| test("consecutive optional dynamic segments in nested routes", () => { | |||
| let manuallyExploded = [ | |||
There was a problem hiding this comment.
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.
| @@ -468,25 +469,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]; | |||
There was a problem hiding this comment.
Prefer the presence of the the param when exploding routes
| .map((exploded) => { | ||
| // for absolute paths, ensure `/` instead of empty segment | ||
| return path.startsWith("/") && exploded === "" ? "/" : exploded; | ||
| }); |
There was a problem hiding this comment.
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]
Found an issue with >2 optional nested segments due to the ordering in which we recombined the exploded paths.
The setup causing this was:
When we had 2 levels of nesting (which we have tests for) everythings fine. But once you get a third level and we get another level of recursion, we were incorrectly combining the results such that child routes interspersed prior to parent routes and we got
/dashboard/:widget2coming before/dashboard/:widget1