fix(router-core): optional-path-param usage variation matches #5176
Conversation
WalkthroughAdds comprehensive optional-parameter examples and tests for React and Solid, introduces many new file-based optional-params routes, removes the React generated routeTree.gen.ts, updates Solid's generated route tree, and changes core path matching to a stateful lookahead-driven, recursive algorithm for optional segments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Router
participant Matcher as PathMatcher
participant Lookahead as OptionalLookahead
App->>Router: navigate(to, params)
Router->>Matcher: isMatch(routePattern, urlSegments)
Matcher->>Lookahead: evaluate current optional (processedOptionals state)
alt lookahead finds viable future match
Note right of Lookahead #DFF2E1: bind optional param and advance
Lookahead-->>Matcher: matched (advance both)
Matcher->>Matcher: continue matching remaining segments
else lookahead cannot satisfy later segments
Note right of Lookahead #FFF3D9: skip optional (advance route index, may recurse)
Lookahead-->>Matcher: skipped (possibly recurse into isMatch)
Matcher->>Matcher: recurse/continue matching remaining segments
end
Matcher-->>Router: match result + resolved params
Router-->>App: render matched component
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
github-actions
bot
added
documentation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 2m 47s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 4s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-09-21 13:37:40 UTC
nlynzaad
changed the title
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/path.ts (1)
758-770: Fix lookahead bugs: wrong base slice and param side‑effects; keep optional counters consistent
- The lookahead recursion slices baseSegments using lookAhead (a route index), which desynchronizes route/base alignment. Use baseIndex (or baseIndex + (lookAhead - routeIndex) if you advance base) instead.
- isMatch recursion mutates the live params object during speculative lookahead. Pass a scratch object to avoid polluting params on failed lookaheads.
- When skipping an optional due to missing baseSegment or encountering '/', processedOptionals is not incremented, skewing remainingOptionals and routeSegmentLength math for subsequent decisions.
Apply this minimal patch:
@@ - if (!baseSegment) { - // No base segment for optional param - skip this route segment - routeIndex++ - continue - } + if (!baseSegment) { + // No base segment for optional param - skip this route segment + processedOptionals++ + routeIndex++ + continue + } @@ - if (baseSegment.value === '/') { - // Skip slash segments for optional params - routeIndex++ - continue - } + if (baseSegment.value === '/') { + // Skip slash segments for optional params + processedOptionals++ + routeIndex++ + continue + } @@ - const remainingRouteSegments = routeSegments.slice( - lookAhead + 1, - ) + const remainingRouteSegments = routeSegments.slice(lookAhead + 1) @@ - const remainingBaseSegments = baseSegments.slice(lookAhead) + // Align to the current base position; skipping optionals consumes no base segments + const remainingBaseSegments = baseSegments.slice(baseIndex) @@ - isMatchedFurtherDown = - remainingRouteSegmentLength === - remainingBaseSegments.length && - isMatch( - remainingBaseSegments, - remainingRouteSegments, - params, - fuzzy, - caseSensitive, - ) + { + const scratchParams: Record<string, string> = {} + isMatchedFurtherDown = + remainingRouteSegmentLength === + remainingBaseSegments.length && + isMatch( + remainingBaseSegments, + remainingRouteSegments, + scratchParams, + fuzzy, + caseSensitive, + ) + } @@ - } else { - skippedOptionals++ - } + } else { + skippedOptionals++ + } processedOptionals++These fixes prevent false negatives/positives in optional chains and stop accidental param leakage during speculative checks.
Also applies to: 805-812, 821-836, 850-905, 919-924
🧹 Nitpick comments (16)
packages/router-core/src/path.ts (1)
821-836: Comment/code mismatch (nit): “bail out” vs behaviorThe comment says we “bail out” on a non-matching future pathname, but the code just breaks the lookahead and proceeds (often correctly matching the optional). Either adjust the comment or add an explicit decision branch.
packages/router-core/tests/match-by-path.test.ts (1)
588-595: Non‑nested case: missing “_” in route tokenThe last entry uses
/B/instead of/B_/, inconsistent with the non‑nested pattern used elsewhere. Likely a typo that weakens the assertion.- ['/a/1/b/2', '/A_/{-$id}_/B/{-$id}_', { id: '2' }], + ['/a/1/b/2', '/A_/{-$id}_/B_/{-$id}_', { id: '2' }],e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
12-15: Trailing-slash mismatch in heading text.Path is defined with a trailing slash (
/path/), but the heading omits it. Align to avoid brittle e2e assertions.Apply this diff:
- Hello "/optional-params/withIndex/-$id/$category/path"! + Hello "/optional-params/withIndex/-$id/$category/path/"!e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
14-17: Placeholder typo: missing “-” before $slug in heading.The route uses
{-$slug}but the heading shows$slug. Fix for consistency and to prevent e2e flakiness.Apply this diff:
- "/optional-params/withRequiredInBetween/-$id/$category/path/$slug"! + "/optional-params/withRequiredInBetween/-$id/$category/path/-$slug"!e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
12-15: Trailing-slash mismatch in heading text.Path is
/path/but heading shows/path. Align for consistency with Solid and to avoid brittle tests.Apply this diff:
- Hello "/optional-params/withIndex/-$id/$category/path"! + Hello "/optional-params/withIndex/-$id/$category/path/"!docs/router/framework/react/guide/path-params.md (5)
137-151: Fix grammar and example comments; clarify wildcard vs prefixed param
- “combines” → “combine”
- Align file path comments with route code.
- Clarify that
postIdis captured by the prefixed param and_splatcaptures the trailing wildcard.Apply:
-You can even combines prefixes with wildcard routes to create more complex patterns: +You can even combine prefixes with wildcard routes to create more complex patterns: @@ -// src/routes/on-disk/storage-{$} -export const Route = createFileRoute('/on-disk/storage-{$postId}/$')({ +// src/routes/on-disk/storage-{$postId}/$.tsx +export const Route = createFileRoute('/on-disk/storage-{$postId}/$')({ @@ -function StorageComponent() { - const { _splat } = Route.useParams() - // _splat, will be value after 'storage-' - // i.e. my-drive/documents/foo.txt - return <div>Storage Location: /{_splat}</div> -} +function StorageComponent() { + const { postId, _splat } = Route.useParams() + // postId is the value after 'storage-' + // _splat is the remaining path (e.g. my-drive/documents/foo.txt) + return ( + <div> + <div>Drive: {postId}</div> + <div>Storage Location: /{_splat}</div> + </div> + ) +}
155-168: Tighten suffix example wording and fix file path comment
- Grammar: “match a URL a filename” → “match a URL for a filename”
- The comment path should include the dot to match the code.
-Suffixes are defined by placing the suffix text outside the curly braces after the variable name. For example, if you want to match a URL a filename that ends with `txt`, you can define it like this: +Suffixes are defined by placing the suffix text outside the curly braces after the variable name. For example, if you want to match a URL for a filename that ends with `.txt`, you can define it like this: @@ -// src/routes/files/{$fileName}txt +// src/routes/files/{$fileName}.txt
173-182: Make wildcard + suffix example consistentUse the same
fileNameparam in the comment as in the code.-// src/routes/files/{$}[.]txt +// src/routes/files/{$fileName}[.]txt
187-200: Align “prefix + suffix” example with descriptionThe text mentions suffix
.jsonbut the example usesperson. Prefer.jsonfor consistency.-// src/routes/users/user-{$userId}person -export const Route = createFileRoute('/users/user-{$userId}person')({ +// src/routes/users/user-{$userId}.json +export const Route = createFileRoute('/users/user-{$userId}.json')({ @@ - // userId will be the value between 'user-' and 'person' + // userId will be the value between 'user-' and '.json'
342-351: Avoid duplicate hook callsDestructure both values from a single
useParams()call.-function DocsComponent() { - const { version } = Route.useParams() - const { _splat } = Route.useParams() +function DocsComponent() { + const { version, _splat } = Route.useParams()e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
10-12: Invalid list semantics:has non-
- direct children
<div>and<hr>under<ul>violate HTML semantics and can hinder accessibility. Either wrap those sections in<li>or replace<ul>with a non-list container.Minimal change (keeps layout, reduces a11y impact):
- <ul class="grid mb-2"> + <div role="list" class="grid mb-2"> @@ - <hr /> + <hr /> @@ - <hr /> + <hr /> @@ - <hr /> + <hr /> @@ - </ul> + </div>If you want full semantic correctness, convert headings and section wrappers into
<li>items instead.Also applies to: 53-54, 105-107, 149-151, 193-197, 238-241
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
10-12: Same list semantics issue as Solid
<ul>has<div>and<hr>as direct children. Mirror the Solid fix here.- <ul className="grid mb-2"> + <div role="list" className="grid mb-2"> @@ - <hr /> + <hr /> @@ - <hr /> + <hr /> @@ - <hr /> + <hr /> @@ - </ul> + </div>Also applies to: 53-54, 105-107, 149-151, 193-197, 238-241
e2e/solid-router/basic-file-based/tests/optionalParams.spec.ts (2)
34-41: Preferexpect(page).toHaveURL(...)overwaitForURL(...)
toHaveURLis more idiomatic, includes built-in waiting, and yields clearer errors. Same applies to other URL checks in this file.Example:
- await page.waitForURL('/optional-params/simple') - pagePathname = new URL(page.url()).pathname - expect(pagePathname).toBe('/optional-params/simple') + await expect(page).toHaveURL('/optional-params/simple')Also applies to: 43-51, 52-60, 61-69
56-58: Stabilize assertions and reduce flake
- Consider
toBeVisible()instead oftoBeInViewport()for reliability across CI environments.- You can DRY repeated patterns with small helpers (e.g.,
assertUrlAndHeading(url, headingTestId)).I can provide a tiny helper module to de-duplicate these patterns if you want.
Also applies to: 111-115, 129-132, 205-216, 404-414
e2e/react-router/basic-file-based/tests/optionalParams.spec.ts (2)
34-41: UsetoHaveURLin place ofwaitForURLSame reasoning as the Solid suite; simplifies and clarifies expectations.
- await page.waitForURL('/optional-params/simple') - pagePathname = new URL(page.url()).pathname - expect(pagePathname).toBe('/optional-params/simple') + await expect(page).toHaveURL('/optional-params/simple')Also applies to: 43-51, 52-60, 61-69
56-58: Visibility assertion and helper extractionPrefer
toBeVisible()overtoBeInViewport()for headings; consider extracting repeated steps.Happy to draft a small test util with
navigateAndAssert(url, headingTestId, paramsTestId, expectedParams)to cut repetition.Also applies to: 111-115, 129-132, 205-216, 404-414
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/router/framework/react/guide/path-params.md(1 hunks)e2e/react-router/basic-file-based/src/routeTree.gen.ts(16 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx(1 hunks)e2e/react-router/basic-file-based/tests/optionalParams.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(16 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/optionalParams.spec.ts(1 hunks)packages/router-core/src/path.ts(3 hunks)packages/router-core/tests/match-by-path.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (2)
e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (3)
e2e/solid-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx (7)
e2e/solid-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (2)
e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx (7)
e2e/react-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (2)
e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (2)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (4)
e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
Route(3-7)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (7)
e2e/react-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx (1)
Route(3-7)
e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (4)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
Route(3-7)
e2e/react-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (2)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)
packages/router-core/tests/match-by-path.test.ts (1)
packages/router-core/src/path.ts (1)
matchByPath(563-603)
e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx (7)
e2e/solid-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
Route(3-7)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx (1)
Route(3-7)
e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (2)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.path.index.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routeTree.gen.ts (1)
e2e/react-router/basic-file-based/src/routeTree.gen.ts (5)
FileRoutesByFullPath(525-601)FileRoutesByTo(602-675)FileRoutesById(676-757)FileRouteTypes(758-992)RootRouteChildren(993-1030)
e2e/react-router/basic-file-based/src/routeTree.gen.ts (1)
e2e/solid-router/basic-file-based/src/routeTree.gen.ts (5)
FileRoutesByFullPath(465-532)FileRoutesByTo(533-597)FileRoutesById(598-670)FileRouteTypes(671-878)RootRouteChildren(879-907)
e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (2)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
Route(3-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (25)
e2e/solid-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
3-7: Solid route and params usage look goodCorrect path shape, and using Route.useParams() as an accessor is correct in Solid. Test IDs/readout are consistent with the e2e tests.
Also applies to: 9-24
packages/router-core/tests/match-by-path.test.ts (1)
51-66: Broad additions look solidNew cases comprehensively exercise optional-after-required, consecutive optionals, and trailing slash handling. These should catch regressions introduced by the lookahead logic.
Please run the updated suite against the patched isMatch to ensure no expectation changes are required.
Also applies to: 83-239, 241-288, 321-335, 348-417
e2e/react-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
3-7: React route is correct; params usage matches React APIPath shape and index route (trailing slash) look right. Using Route.useParams() as an object is correct in React.
Also applies to: 9-17
e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
3-5: Solid simple/path route LGTMCorrect accessor usage and test IDs align with the suite.
Also applies to: 7-15
e2e/solid-router/basic-file-based/src/routes/optional-params/withIndex/{-$id}.$category.index.tsx (1)
9-17: LGTM (Solid index route): params accessor usage is correct.
Route.useParams()is used as an accessor and the test IDs look consistent with the pattern.e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
7-14: LGTM (React index route): params object usage is correct.Hook usage and test IDs look consistent with the simple optional pattern.
e2e/solid-router/basic-file-based/src/routes/optional-params/simple/{-$id}.index.tsx (1)
7-14: LGTM (Solid index route): params accessor usage is correct.Accessor call and test IDs are consistent with the simple pattern.
e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredInBetween/{-$id}.$category.path.{-$slug}.tsx (1)
3-7: Solid route component and params usage look correctRoute definition, test IDs, and Solid’s
Route.useParams()accessor usage (params()) align with the e2e tests.Also applies to: 9-26
e2e/react-router/basic-file-based/src/routes/optional-params/consecutive/{-$id}.{-$slug}.$category.info.tsx (1)
3-7: React route wiring and params rendering LGTMCorrect
createFileRoutepath, test IDs, andRoute.useParams()usage (object, not accessor) match the tests.Also applies to: 9-24
e2e/react-router/basic-file-based/src/routes/optional-params/simple/{-$id}.path.tsx (1)
3-15: Route and test IDs are correctMatches e2e expectations;
Route.useParams()(object) usage is appropriate.docs/router/framework/react/guide/path-params.md (1)
414-420: Type note — numeric params are allowed (auto‑stringified)Confirmed: Link/To accept params: Record<string, unknown>, so params={{ category: 123 }} type‑checks and will be auto‑stringified for the path; parsed URL params remain strings.
e2e/react-router/basic-file-based/src/routeTree.gen.ts (7)
24-24: Optional-params imports wired correctly.All new route imports resolve to expected paths under routes/optional-params/.
Also applies to: 77-77, 79-79, 86-86, 88-92
149-153: Registering the optional-params route group looks correct.id, path, and parent match the new group.
431-437: Child routes for optional-param patterns are consistent and parented correctly.Paths/ids map to the described patterns; trailing-slash conventions match the generator’s norms.
Also applies to: 442-447, 483-488, 494-499, 500-507, 508-515, 516-523
1111-1117: Module augmentation entries added for the group and all children.preLoaderRoute and parentRoute bindings look correct.
Also applies to: 1482-1488, 1496-1502, 1545-1551, 1559-1565, 1566-1572, 1573-1579, 1580-1586
1616-1645: Route children map is complete for the new group.All declared children are exported via OptionalParamsRouteRouteWithChildren.
1852-1854: Root children expose OptionalParams group.Group is now directly navigable from root.
528-528: Verification blocked — permission denied reading generated route files.Automated check couldn’t confirm optional-params across FileRoutesByFullPath/FileRoutesByTo/FileRoutesById in both routers because the script failed with: "/bin/bash: line 57: e2e/react-router/basic-file-based/src/routeTree.gen.ts: Permission denied". Grant read access and re-run the provided script, or paste the three
export interfaceblocks (FileRoutesByFullPath, FileRoutesByTo, FileRoutesById) from these files so I can re-verify: e2e/react-router/basic-file-based/src/routeTree.gen.ts and e2e/solid-router/basic-file-based/src/routeTree.gen.ts.e2e/solid-router/basic-file-based/src/routeTree.gen.ts (7)
23-23: Optional-params imports added.Imports mirror the React variant and match on-disk routes.
Also applies to: 68-83
134-138: Optional-params group registration is correct.Proper id/path under root.
371-376: Child routes for optional-params added with correct parenting and paths.Patterns and trailing-slash handling align with generator behavior.
Also applies to: 382-387, 423-429, 434-439, 440-447, 448-455, 456-463
468-468: Type surfaces (FullPath/To/Id) reflect the new optional-params routes.Coverage looks complete and consistent.
Also applies to: 513-516, 523-524, 528-531, 534-537, 578-581, 588-589, 593-596, 599-600, 602-603, 651-654, 661-669, 672-677
981-987: Module augmentation for @tanstack/solid-router updated.All optional-params entries are present with correct preLoaderRoute bindings.
Also applies to: 1296-1302, 1310-1316, 1359-1365, 1373-1379, 1380-1386, 1387-1393, 1394-1400
1430-1455: Children map and with-children export added for the group.Matches the route constants declared above.
Also applies to: 1457-1459
1664-1667: Root children now include OptionalParams group.Expected for top-level navigation and typing.
...c-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx
Show resolved
Hide resolved
...c-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/path.ts (1)
759-770: Skipping optionals on missing base or slash should bump the counters.When
!baseSegmentorbaseSegment.value === '/', youcontinuewithout updatingprocessedOptionals/skippedOptionals. This desynchronizes later lookahead math.- if (!baseSegment) { - // No base segment for optional param - skip this route segment - routeIndex++ - continue - } + if (!baseSegment) { + // No base segment for optional param - skip this route segment + skippedOptionals++ + processedOptionals++ + routeIndex++ + continue + } - if (baseSegment.value === '/') { - // Skip slash segments for optional params - routeIndex++ - continue - } + if (baseSegment.value === '/') { + // Skip slash segments for optional params + skippedOptionals++ + processedOptionals++ + routeIndex++ + continue + }
🧹 Nitpick comments (1)
packages/router-core/src/path.ts (1)
614-618: Stateful optional counters add footguns; prefer deriving remaining counts per-iteration.
processedOptionals,skippedOptionals, and a globaloptionalCountare easy to desynchronize and are later used with total lengths (see comments below). Consider deriving remaining optionals fromrouteSegments.slice(routeIndex)when needed to avoid global state coupling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx(1 hunks)packages/router-core/src/path.ts(3 hunks)packages/router-core/tests/match-by-path.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx
- e2e/solid-router/basic-file-based/src/routes/optional-params/route.tsx
- e2e/solid-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx
- e2e/react-router/basic-file-based/src/routes/optional-params/withRequiredParam/{-$id}.$category.{-$slug}.info.tsx
- packages/router-core/tests/match-by-path.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/router-core/src/path.ts (2)
821-836: Good early disambiguation on PATHNAME lookahead.Skipping the current optional when the next pathname equals the current base segment is correct and prevents over-greedy optional matches. Ensure both case-sensitive and insensitive test variants exist.
919-924: Counters update order/coverage.Incrementing
skippedOptionalsand thenprocessedOptionalshere is fine for the branch that attempted a match. After applying the earlier fix for the early-continue cases, the invariants will hold across all branches.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router-core/src/path.ts (2)
829-839: Comment/behavior mismatch for PATHNAME lookahead.Comment says “invalid url ⇒ return undefined,” but code just breaks. Either adjust the comment or implement the early failure.
- if ( + const futureMatchesBase = caseSensitive ? futureRouteSegment.value === baseSegment.value : futureRouteSegment.value.toLowerCase() === baseSegment.value.toLowerCase() - ) { - // so we should skip this optional parameter - shouldMatchOptional = false - } - break + if (futureMatchesBase) { + // skip this optional parameter + shouldMatchOptional = false + break + } else { + // no match ahead and a required pathname follows + return false + }
842-852: Consecutive optionals: code contradicts the “prefer first optional” note.The condition sets
shouldMatchOptional = false, effectively preferring a later optional. Either invert the condition or reword the comment to reflect actual intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/path.ts(4 hunks)
🔇 Additional comments (1)
packages/router-core/src/path.ts (1)
883-895: Good: speculative recursion now slices base correctly and clones params.
baseSegments.slice(baseIndex + 1)and{ ...params }avoid index mixups and side-effects. Matches earlier feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/router-core/src/path.ts (5)
614-619: Remove unused skippedOptionals bookkeeping.
skippedOptionalsis never read after being incremented. Drop the variable and its increments to simplify control flow.Apply:
- let processedOptionals = 0 - let skippedOptionals = 0 + let processedOptionals = 0
929-934: Delete dead increment of skippedOptionals.Since
skippedOptionalsis not used, remove this branch to reduce noise.- } else { - skippedOptionals++ - } - - processedOptionals++ + } + processedOptionals++
808-816: Precompute remaining required segments for clarity; current math is correct.The routeIndex-based remaining length fixes prior mid-route skew. For readability, derive “remaining required” once and reuse.
- const remainingOptionals = optionalCount - processedOptionals - 1 > 0 - // consider last route segment might be index route and any prior optionals that was not matched - const remainingRouteSegmentLength = + const remainingOptionals = optionalCount - processedOptionals - 1 > 0 + // consider last route segment might be index route and any prior optionals that were not matched + const remainingRouteSegmentLength = (routeSegments.slice(-1)[0]?.value === '/' ? routeSegments.length - 1 : routeSegments.length) - routeIndex + const remainingRequiredSegmentsFromHere = + remainingRouteSegmentLength - (optionalCount - processedOptionals)
842-854: Reuse the derived “remaining required” metric.Logic is sound; switching to the precomputed variable reduces cognitive load.
- if ( - remainingRouteSegmentLength - - optionalCount + - processedOptionals >= - baseSegments.length - baseIndex - ) { + if (remainingRequiredSegmentsFromHere >= (baseSegments.length - baseIndex)) { shouldMatchOptional = false }
880-885: Avoid shadowing remainingRouteSegmentLength — rename the inner declaration and update its local uses.packages/router-core/src/path.ts contains two const declarations named
remainingRouteSegmentLength(decls at ~line 812 and ~line 880). The inner one shadows the outer and is used nearby (comparisons around lines 888, 902–904, 910–911). Rename the inner variable (e.g.remainingRouteSegmentLengthAfterFollowing) and update those local comparisons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/path.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/router-core/src/path.ts (3)
758-766: Early-exit handling for missing//base segments looks correct.Incrementing
processedOptionalsand advancingrouteIndexhere avoids infinite loops and correctly treats absent or slash segments as “skipped optional.”Also applies to: 768-774
825-839: Good: prefer skipping optional when next static segment matches current base.Case sensitivity is honored; early break prevents overconsumption by the optional.
856-897: Nice fixes: safe speculative recursion and correct base slicing.Cloning
paramsand usingbaseIndex + 1forremainingBaseSegmentsaddress the prior mutation and index bugs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/path.ts (1)
900-912: Avoid shadowing “remainingRouteSegmentLength” — rename to prevent misreads.The inner block defines a
remainingRouteSegmentLengththat shadows the outer one; later checks (Line 908) refer to the outer value. Rename for clarity to prevent future regressions.Apply:
- const remainingRouteSegmentLength = + const remainingRouteLenFromHere = (routeSegments.slice(-1)[0]?.value === '/' ? routeSegments.length - 1 : routeSegments.length) - routeIndex - const remainingRequiredRouteSegmentCount = - remainingRouteSegmentLength - (optionalCount - processedOptionals) + const remainingRequiredRouteCount = + remainingRouteLenFromHere - (optionalCount - processedOptionals) const remainingBaseSegmentCount = baseSegments.length - baseIndexAnd below:
- if ( - remainingRequiredRouteSegmentCount >= remainingBaseSegmentCount - ) { + if (remainingRequiredRouteCount >= remainingBaseSegmentCount) { shouldMatchOptional = false }Keep the inner-scope var distinct too:
- const remainingRouteSegmentLength = + const followingRemainingRouteLen = remainingRouteSegments.slice(-1)[0]?.value === '/' ? remainingRouteSegments.length - 1 : remainingRouteSegments.length ... - isMatchedFurtherDown = - remainingRouteSegmentLength === + isMatchedFurtherDown = + followingRemainingRouteLen === remainingBaseSegments.length && isMatch(And finally:
- if (remainingBaseSegmentCount < remainingRouteSegmentLength) { + if (remainingBaseSegmentCount < remainingRouteLenFromHere) { shouldMatchOptional = false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/path.ts(4 hunks)
🔇 Additional comments (5)
packages/router-core/src/path.ts (5)
614-619: Optional-segment bookkeeping added — sane approach.Tracking
processedOptionalsand the precomputedoptionalCountis clear and avoids repeated scans. Looks good.
758-766: Skip paths correctly advance optional counters.Incrementing
processedOptionalsbeforecontinuepreserves invariants when base is exhausted or a/is encountered. Good.
809-821: Remaining-length math now scoped to current indices — resolves prior miscounts.Using
(routeSegments.length - routeIndex)and(baseSegments.length - baseIndex)(with index-route adjustment) fixes the earlier global-length bug and makes optional decisions correct mid-route. Also like the early break on static PATHNAME and the consecutive-optionals guard.Also applies to: 829-855
857-899: Speculative probe no longer mutates caller and slices base correctly.Cloning
paramsand usingbaseSegments.slice(baseIndex + 1)address the prior side-effects/off-by-index issues in lookahead recursion. Nice.
928-929: Post‑processing increment ensures consistent optional accounting.Incrementing
processedOptionalsafter match-or-skip keeps the remaining-optionals math correct for subsequent lookahead. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/tests/match-by-path.test.ts (1)
644-658: Replace stray '/a/{-$id}/b_/{-$other}' with '/a/{-$id}/b/{-$other}_' (packages/router-core/tests/match-by-path.test.ts:651)Single occurrence at line 651; apply patch:
- ['/a/b', '/a/_{-$id}_/b_/{-$other}_', {}], + ['/a/b', '/a_/{-$id}_/b_/{-$other}_', {}],
🧹 Nitpick comments (2)
packages/router-core/tests/match-by-path.test.ts (2)
55-66: Add reciprocal 'from' trailing-slash strictness cases for optionalsGood additions for trailing slashes on the pattern. Add the inverse (trailing slash on the URL but not on the pattern) to lock strict behavior.
Apply this diff inside the same dataset:
+ ['/a/1/b/', '/a/{-$id}/b', undefined], + ['/a/b/', '/a/{-$id}/b', undefined], + ['/a/1/b/2/', '/a/{-$id}/b/{-$other}', undefined], + ['/a/b/2/', '/a/{-$id}/b/{-$other}', undefined],
83-239: Broad coverage looks solid; consider DRYing trailing-slash twinsThe permutations are thorough. To reduce repetition/typos, consider a small helper to auto-generate to + to/ variants for each case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/tests/match-by-path.test.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/tests/match-by-path.test.ts (1)
packages/router-core/src/path.ts (1)
matchByPath(563-603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
packages/router-core/tests/match-by-path.test.ts (4)
241-288: Consecutive optionals: coverage LGTMThese cases accurately exercise presence/absence across multiple optionals and adjacent literals.
321-331: Case-insensitive optionals: parity with strict mode looks correctThe added cases align with expectations in case-insensitive mode.
348-417: Case-insensitive complex optionals: thorough and consistentGood mix of optional + required + trailing-slash variants; expectations read correctly.
594-594: Non-nested, case-insensitive repeated param override: OKConfirms later
$idwins; matches behavior in other suites.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/react-router/basic-file-based/src/routes/optional-params/single/{-$id}.tsx (1)
11-13: Minor: align heading literal with route syntaxShow the parameter with braces for consistency with file-based pattern.
- Hello "/optional-params/single/-$id"! + Hello "/optional-params/single/{-$id}"!e2e/react-router/basic-file-based/tests/optionalParams.spec.ts (4)
320-323: Stability: prefer visibility over viewport checkstoBeInViewport can be flaky; toBeVisible is sufficient here.
- await expect( - page.getByTestId('withRequiredInBetween-heading'), - ).toBeInViewport() + await expect( + page.getByTestId('withRequiredInBetween-heading'), + ).toBeVisible()
337-340: Same as above: swap viewport assertion- await expect( - page.getByTestId('withRequiredInBetween-heading'), - ).toBeInViewport() + await expect( + page.getByTestId('withRequiredInBetween-heading'), + ).toBeVisible()
355-357: Same as above: swap viewport assertion- ).toBeInViewport() + ).toBeVisible()
371-374: Same as above: swap viewport assertion- ).toBeInViewport() + ).toBeVisible()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-router/basic-file-based/src/routeTree.gen.ts(0 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/single/path.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/optional-params/single/{-$id}.tsx(1 hunks)e2e/react-router/basic-file-based/tests/optionalParams.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/react-router/basic-file-based/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/react-router/basic-file-based/src/routes/optional-params/single/{-$id}.tsx (2)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/single/path.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/optional-params/single/path.tsx (2)
e2e/react-router/basic-file-based/src/routes/optional-params/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/optional-params/single/{-$id}.tsx (1)
Route(3-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
e2e/react-router/basic-file-based/src/routes/optional-params/single/path.tsx (1)
3-5: LGTM: route export and wiring look correctMatches the test usage and naming conventions.
e2e/react-router/basic-file-based/src/routes/optional-params/single/{-$id}.tsx (1)
3-5: LGTM: file-route definitionRoute creation and params usage are correct.
|
|
||
| // consider last route segment might be index route and any prior optionals that was not matched | ||
| const remainingRouteSegmentLength = | ||
| (routeSegments.slice(-1)[0]?.value === '/' |
There was a problem hiding this comment.
isMatch is a critical part of the router, we should be careful about performance. Maybe we can use the last() utility function to access the last item in an array instead of this?
| (routeSegments.slice(-1)[0]?.value === '/' | |
| (last(routeSegments)?.value === '/' |
There was a problem hiding this comment.
thanks. applied this. agreed performance here is paramount hence added extra early exits from the for loop running the isMatch only in specific circumstances
| isMatch( | ||
| remainingBaseSegments, | ||
| remainingRouteSegments, | ||
| { ...params }, |
There was a problem hiding this comment.
I don't think it needs to inherit params
| { ...params }, | |
| {}, |
There was a problem hiding this comment.
params gets modified in isMatch when the match is found, for this here we only want to confirm a match exists and don't want it to mutate the params, hence the spread.
There was a problem hiding this comment.
but it's never reading from it either right? So if we're not gonna use it, and it doesn't need it, why spread at all? Why not just pass in an empty object?
|
I also encountered this problem today |
|
The added tests has highlighted additional issues with optional params, unrelated to this PR, that we need to address. Unfortunately I have been busy dealing with other issues on non-nested paths that has taken my attention away from this PR. I hope to return to this as soon as we have resolved those. |
#4933 highlighted a case where index routes would not resolve when an optional param is followed by a required path param. this pr resolves #4933
apart from this specific issue, this PR updates isMatch to unlock some more variations in the usage of optional params including:
{-$optional}.$pathParam.index
{-$optional}.{-$optional}.$pathParam.path (with and without index)
{-$optional}.$pathParam.{-$optional}.path (with and without index)
{-$optional}.path.{-$optional}.$pathParam (with and without index)
{-$optional}.path.$pathParam.{-$optional} (with and without index)
etc.
bunch of unit tests added to match-path tests to test various variations and completions.
also added e2e tests on react and solid to test functionality of this. not all variations are covered here since they are covered in the unit test. this is just to ensure findings from unit tests carry over to e2e.
seperate PR to follow to merge into alpha
Summary by CodeRabbit
New Features
Documentation
Tests