Skip to content

fix: only allow leaf nodes as to suggestions #1495

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 1 commit into from
Apr 19, 2024
Merged

fix: only allow leaf nodes as to suggestions #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 13 additions & 4 deletions packages/react-router/src/link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { Trim } from './fileRoute'
import type { AnyRoute, RootSearchSchema } from './route'
import type {
RouteByPath,
RouteLeaves,
RoutePaths,
RoutePathsAutoComplete,
} from './routeInfo'
Expand Down Expand Up @@ -88,8 +89,8 @@ export type RemoveLeadingSlashes<T> = T extends `/${infer R}`

export type ResolvePaths<TRouteTree extends AnyRoute, TSearchPath> =
RouteByPath<TRouteTree, RemoveTrailingSlashes<TSearchPath>> extends never
? RoutePaths<TRouteTree>
: RoutePaths<RouteByPath<TRouteTree, RemoveTrailingSlashes<TSearchPath>>>
? RouteLeaves<TRouteTree>
: RouteLeaves<RouteByPath<TRouteTree, RemoveTrailingSlashes<TSearchPath>>>

export type SearchPaths<
TRouteTree extends AnyRoute,
Expand Down Expand Up @@ -137,7 +138,7 @@ export type AbsolutePathAutoComplete<
? never
: './')
| (string extends TFrom ? '../' : TFrom extends `/` ? never : '../')
| RoutePaths<TRouteTree>
| RouteLeaves<TRouteTree>
| (TFrom extends '/' ? never : SearchPaths<TRouteTree, TFrom>)

export type RelativeToPathAutoComplete<
Expand Down Expand Up @@ -432,7 +433,15 @@ export type LinkOptions<
}

export type CheckPath<TRouteTree extends AnyRoute, TPass, TFail, TFrom, TTo> =
ResolveRoute<TRouteTree, TFrom, TTo> extends never ? TFail : TPass
ResolveRoute<TRouteTree, TFrom, TTo> extends infer TRoute extends AnyRoute
? [TRoute] extends [never]
? TFail
Comment on lines +437 to +438
Copy link
Contributor

@schiller-manuel schiller-manuel Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is this really necessary?
I removed it, and type tests still work (and it reduces instantiations from 573059 to 572245)

Copy link
Contributor Author

@chorobin chorobin Apr 19, 2024

Choose a reason for hiding this comment

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

You're right atm and thats because we're not strictly checking from. But this first checks if the route exists at all and if it does not then fail.

: string extends TTo
? TPass
: unknown extends TRoute['children']
? TPass
: TFail
: TFail

export type ResolveRelativePath<TFrom, TTo = '.'> = TFrom extends string
? TTo extends string
Expand Down
13 changes: 11 additions & 2 deletions packages/react-router/src/routeInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import type { AnyRoute } from './route'
import type { Expand, UnionToIntersection, UnionToTuple } from './utils'
import type { UnionToIntersection, UnionToTuple } from './utils'

export type ParseRoute<TRouteTree, TAcc = TRouteTree> = TRouteTree extends {
types: { children: infer TChildren }
}
? TChildren extends ReadonlyArray<unknown>
? TChildren extends ReadonlyArray<any>
? ParseRoute<TChildren[number], TAcc | TChildren[number]>
: TAcc
: TAcc

export type RouteLeaves<TRouteTree> =
ParseRoute<TRouteTree> extends infer TRoute extends AnyRoute
? TRoute extends any
? TRoute['types']['children'] extends ReadonlyArray<any>
? never
: TRoute['fullPath']
: never
: never

export type RoutesById<TRouteTree extends AnyRoute> = {
[K in ParseRoute<TRouteTree> as K['id']]: K
}
Expand Down
41 changes: 24 additions & 17 deletions packages/react-router/tests/link.test-d.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,14 @@ test('when navigating to the root', () => {
| './'
| ''
| '/'
| '/invoices'
| '/invoices/'
| '/invoices/$invoiceId'
| '/invoices/$invoiceId/details'
| '/invoices/$invoiceId/details/$detailId'
| '/invoices/$invoiceId/edit'
| '/posts'
| '/posts/'
| '/posts/$postId'
| 'invoices'
| 'invoices/'
| 'invoices/$invoiceId'
| 'invoices/$invoiceId/details'
| 'invoices/$invoiceId/details/$detailId'
| 'invoices/$invoiceId/edit'
| 'posts'
| 'posts/'
| 'posts/$postId'
| undefined
Expand All @@ -110,13 +102,9 @@ test('when navigating from a route with no params and no search to the root', ()
| './'
| '/'
| ''
| '/invoices'
| '/invoices/'
| '/invoices/$invoiceId'
| '/invoices/$invoiceId/details'
| '/invoices/$invoiceId/edit'
| '/invoices/$invoiceId/details/$detailId'
| '/posts'
| '/posts/'
| '/posts/$postId'
| '$postId'
Expand All @@ -136,20 +124,39 @@ test('when navigating from a route with no params and no search to the parent ro
.parameter(0)
.toHaveProperty('to')
.toEqualTypeOf<
| '../posts'
| '../posts/'
| '../posts/$postId'
| '../invoices/$invoiceId'
| '../invoices/$invoiceId/edit'
| '../invoices/$invoiceId/details'
| '../invoices/$invoiceId/details/$detailId'
| '../invoices'
| '../invoices/'
| '../'
| undefined
>()
})

test('cannot navigate to a branch', () => {
expectTypeOf(Link<RouteTree, string, '/invoices/invoiceId'>)
.parameter(0)
.toHaveProperty('to')
.toEqualTypeOf<
| ''
| '../'
| './'
| '/'
| '/invoices/'
| '/invoices/$invoiceId/details/$detailId'
| '/invoices/$invoiceId/edit'
| '/posts/'
| '/posts/$postId'
| 'invoices/'
| 'invoices/$invoiceId/details/$detailId'
| 'invoices/$invoiceId/edit'
| 'posts/'
| 'posts/$postId'
| undefined
>()
})

test('from autocompletes to all absolute routes', () => {
const TestLink = Link<RouteTree, '/', '/'>
expectTypeOf(TestLink)
Expand Down Expand Up @@ -248,7 +255,7 @@ test('when navigating to a route with params', () => {
})

test('when navigating from a route with no params to a route with params', () => {
const TestLink = Link<RouteTree, '/invoices/', './$invoiceId/'>
const TestLink = Link<RouteTree, '/invoices/', './$invoiceId/edit'>

expectTypeOf(TestLink).parameter(0).toMatchTypeOf<{ params: unknown }>()

Expand Down