Skip to content
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.

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

Trim parent route id prefix when creating lazy routes #2401

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
13 changes: 10 additions & 3 deletions packages/react-router/src/fileRoute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { useLoaderData } from './useLoaderData'
import { useSearch } from './useSearch'
import { useParams } from './useParams'
import { useNavigate } from './useNavigate'
import { getLastPathSegment } from './path'
import type { Constrain } from './utils'
import type {
AnyContext,
Expand All @@ -21,7 +22,7 @@ import type {
UpdatableRouteOptions,
} from './route'
import type { MakeRouteMatch } from './Matches'
import type { AnyRouter, RegisteredRouter } from './router'
import type { RegisteredRouter } from './router'
import type { RouteById, RouteIds } from './routeInfo'

export interface FileRoutesByPath {
Expand Down Expand Up @@ -261,7 +262,10 @@ export function createLazyRoute<
TRoute extends AnyRoute = RouteById<RegisteredRouter['routeTree'], TId>,
>(id: TId) {
return (opts: LazyRouteOptions) => {
return new LazyRoute<TRoute>({ id: id as any, ...opts })
return new LazyRoute<TRoute>({
id: getLastPathSegment(id as any) as any,
...opts,
})
}
}

Expand All @@ -276,5 +280,8 @@ export function createLazyFileRoute<
TRoute extends FileRoutesByPath[TFilePath]['preLoaderRoute'],
>(id: TFilePath) {
return (opts: LazyRouteOptions) =>
new LazyRoute<TRoute>({ id: removeGroups(id), ...opts })
new LazyRoute<TRoute>({
id: getLastPathSegment(removeGroups(id)) as any,
Copy link
Contributor

Choose a reason for hiding this comment

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

i am wondering about why the last path segment should be used as id.
this might work, but aren't there cases where this is not unique?

e.g. consider the following routes:

/foo/bar
/hello/bar

both would end up with the same id, right?

Copy link
Author

@stevenlyd stevenlyd Sep 25, 2024

Choose a reason for hiding this comment

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

Yes, you're right that both /foo/bar and /hello/bar have the same last path segment bar. However, this only affects the options.id of the lazy route object. From what I understand, the options.id in the lazy route object is mainly used during the preloading of a route. When a route is preloaded, these options are assigned back to the primary route object like this:

// ./packages/react-router/src/router.ts
route._lazyPromise =
  route._lazyPromise ||
  (route.lazyFn
    ? route.lazyFn().then((lazyRoute) => {
        Object.assign(route.options, lazyRoute.options)
      })
    : Promise.resolve())

I believe that the options.id should represent the route's own ID, not the full ID that includes prefixes from parent routes. This seems to be consistent with how IDs and paths got updated for primary route objects in routeTree.gen.ts. For example:

const ProtectedRouteRoute = ProtectedRouteImport.update({
  id: '/_protected',
  getParentRoute: () => rootRoute,
} as any)

const ProtectedLayoutRouteRoute = ProtectedLayoutRouteImport.update({
  id: '/_layout', // Full id should be /_protected/_layout
  getParentRoute: () => ProtectedRouteRoute,
} as any)

const ProtectedLayoutParentRouteRoute = ProtectedLayoutParentRouteImport.update(
  {
    path: '/parent',
    getParentRoute: () => ProtectedLayoutRouteRoute,
  } as any,
)

const ProtectedLayoutParentTestRouteRoute =
  ProtectedLayoutParentTestRouteImport.update({
    path: '/test', // Full path should be /parent/test
    getParentRoute: () => ProtectedLayoutParentRouteRoute,
  } as any).lazy(() =>
    import('./routes/_protected/_layout/parent/test/route.lazy').then(
      (d) => d.Route,
    ),
  )

I guess the reason why we assign the full ID at the beginning but update it to a trimmed version later is that the code generator needs to consume that full path or full ID information when auto-generating the route tree.

If I am not mistaken, it's precisely because the id and path in primary route objects are updated to their trimmed versions in routeTree.gen.ts that TSR can function properly. Otherwise if they are in full version, the route.id we generate initially would be incorrect, containing duplicate parent prefixes. However, we currently don't trim the options.id for the lazy route object, which can cause issues when it gets assigned back to the primary route object.

Of course, these incorrect options.id values only take effect when we run router.buildRouteTree again. Since few people rerun router.buildRouteTree after the router is instantiated, this issue hasn't been discovered until now.

From my understanding, route.options.id represents the route's own ID, whereas route.id is the full ID that exists only in the primary route object and is generated when route.init is called. This process combines the customId with the parent's full ID as a prefix. Additionally, the customId is derived from either the path (not fullPath) or options.id, depending on whether the route is pathless.

As a result, the route's ID ultimately become /foo/bar and /hello/bar, maintaining uniqueness even if the last segments are the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test for this as well please?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, you mean a test that checks if the lazy route object getting the trimmed version of id?

Copy link
Contributor

Choose a reason for hiding this comment

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

and that IDs are still unique then

Copy link
Author

Choose a reason for hiding this comment

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

and that IDs are still unique then

I've added tests for route ID uniqueness, and also added routes ending with same last segments to that test router.

Copy link
Author

Choose a reason for hiding this comment

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

and that IDs are still unique then

And also added tests that check if the lazy route's options.id is trimmed.

...opts,
})
}
9 changes: 9 additions & 0 deletions packages/react-router/src/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ export function removeTrailingSlash(value: string, basepath: string): string {
return value
}

export function getLastPathSegment(path: string) {
if (typeof path !== 'string') {
return undefined
}

const match = path.match(/(\/[^/]+)\/?$/)
return match?.[1]
}

// intended to only compare path name
// see the usage in the isActive under useLinkProps
// /sample/path1 = /sample/path1/
Expand Down
4 changes: 2 additions & 2 deletions packages/react-router/src/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type { NavigateOptions, ParsePathParams, ToMaskOptions } from './link'
import type { ParsedLocation } from './location'
import type { RouteById, RouteIds, RoutePaths } from './routeInfo'
import type { AnyRouter, RegisteredRouter, Router } from './router'
import type { Assign, Constrain, Expand, NoInfer, PickRequired } from './utils'
import type { Assign, Constrain, Expand, NoInfer } from './utils'
import type { BuildLocationFn, NavigateFn } from './RouterProvider'
import type { NotFoundError } from './not-found'
import type { LazyRoute } from './fileRoute'
Expand Down Expand Up @@ -1029,7 +1029,7 @@ export class Route<
path = trimPathLeft(path)
}

const customId = options?.id || path
const customId = path || options?.id

// Strip the parentId prefix from the first level of children
let id = isRoot
Expand Down
58 changes: 58 additions & 0 deletions packages/react-router/tests/fileRoute.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,61 @@ describe('createLazyRoute has the same hooks as getRouteApi', () => {
},
)
})

describe('LazyRoute options.id should not include parent segments', () => {
describe('createLazyRoute', () => {
it('should set id to the last path segment without parents', () => {
const id = '/parent_a/parent_b/child'
const route = createLazyRoute(id)({})
expect(route.options.id).toBe('/child')
})

it('should remove group patterns from id', () => {
const idWithGroups = '/parent_a/(group)/child'
const route = createLazyRoute(idWithGroups)({})
expect(route.options.id).toBe('/child')
})

it('should replace multiple slashes with a single slash', () => {
const idWithDoubleSlashes = '/parent_a//child'
const route = createLazyRoute(idWithDoubleSlashes)({})
expect(route.options.id).toBe('/child')
})

it('should not change anything when the id is already trimmed', ()=> {
const normalId = '/child'
const route = createLazyRoute(normalId)({})
expect(route.options.id).toBe('/child')
})
})

describe('createLazyFileRoute', () => {
it('should set id to the last path segment without parents', () => {
const id = '/parent_a/parent_b/child'
// @ts-expect-error
const route = createLazyFileRoute(id)({})
expect(route.options.id).toBe('/child')
})

it('should remove group patterns from id', () => {
const idWithGroups = '/parent_a/(group)/child'
// @ts-expect-error
const route = createLazyFileRoute(idWithGroups)({})
expect(route.options.id).toBe('/child')
})

it('should replace multiple slashes with a single slash', () => {
const idWithDoubleSlashes = '/parent_a//child'
// @ts-expect-error
const route = createLazyFileRoute(idWithDoubleSlashes)({})
expect(route.options.id).toBe('/child')
})

it('should not change anything when the id is already trimmed', ()=> {
const normalId = '/child'
// @ts-expect-error
const route = createLazyFileRoute(normalId)({})
expect(route.options.id).toBe('/child')
})
})
})
13 changes: 13 additions & 0 deletions packages/react-router/tests/lazy/normal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { createLazyFileRoute, createLazyRoute } from '../../src'

export function Route(id: string) {
return createLazyRoute(id)({
component: () => <h1>I'm a normal route</h1>,
})
}

export function FileRoute(id: string) {
return createLazyFileRoute(id as any)({
component: () => <h1>I'm a normal file route</h1>,
})
}
83 changes: 83 additions & 0 deletions packages/react-router/tests/path.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, expect, it } from 'vitest'
import {
exactPathTest,
getLastPathSegment,
interpolatePath,
removeBasepath,
removeTrailingSlash,
Expand Down Expand Up @@ -276,3 +277,85 @@ describe('interpolatePath', () => {
})
})
})

describe('getLastPathSegment', () => {
it.each([
{
name: 'should return the last segment with leading slash when path ends with a slash',
path: '/_protected/test/',
expected: '/test',
},
{
name: 'should return the last segment with leading slash when path does not end with a slash',
path: '/_protected/test',
expected: '/test',
},
{
name: 'should return the last segment in a longer path with trailing slash',
path: '/another/path/example/',
expected: '/example',
},
{
name: 'should return the last segment in a longer path without trailing slash',
path: '/another/path/example',
expected: '/example',
},
{
name: 'should return the only segment with leading slash',
path: '/single',
expected: '/single',
},
{
name: 'should return the last segment with leading slash in a nested path',
path: '/trailing/slash/',
expected: '/slash',
},
{
name: 'should return undefined for root path',
path: '/',
expected: undefined,
},
{
name: 'should return undefined for empty string',
path: '',
expected: undefined,
},
{
name: 'should handle paths with numbers correctly',
path: '/users/123/',
expected: '/123',
},
{
name: 'should handle paths with special characters',
path: '/path/with-special_chars!',
expected: '/with-special_chars!',
},
{
name: 'should handle paths with multiple special characters and trailing slash',
path: '/path/with/$pecial-Chars123/',
expected: '/$pecial-Chars123',
},
{
name: 'should return undefined when there is no segment after the last slash',
path: '/path/to/directory//',
expected: undefined,
},
{
name: 'should handle paths without leading slash',
path: 'no/leading/slash',
expected: '/slash',
},
{
name: 'should handle single slash',
path: '/',
expected: undefined,
},
{
name: 'should handle path with multiple trailing slashes',
path: '/multiple/trailing/slashes///',
expected: undefined,
},
])('$name', ({ path, expected }) => {
expect(getLastPathSegment(path)).toBe(expected)
})
})
134 changes: 134 additions & 0 deletions packages/react-router/tests/router.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,54 @@ function createTestRouter(initialHistory?: RouterHistory) {
getParentRoute: () => pathSegmentLayoutSplatRoute,
path: '$',
})
const protectedRoute = createRoute({
getParentRoute: () => rootRoute,
id: '/_protected',
}).lazy(() => import('./lazy/normal').then((f) => f.Route('/_protected')))
const protectedLayoutRoute = createRoute({
getParentRoute: () => protectedRoute,
id: '/_layout',
}).lazy(() =>
import('./lazy/normal').then((f) => f.Route('/_protected/_layout')),
)
const protectedFileBasedLayoutRoute = createRoute({
getParentRoute: () => protectedRoute,
id: '/_fileBasedLayout',
}).lazy(() =>
import('./lazy/normal').then((f) =>
f.FileRoute('/_protected/_fileBasedLayout'),
),
)
const protectedFileBasedLayoutParentRoute = createRoute({
getParentRoute: () => protectedFileBasedLayoutRoute,
path: '/fileBasedParent',
}).lazy(() =>
import('./lazy/normal').then((f) =>
f.FileRoute('/_protected/_fileBasedLayout/fileBasedParent'),
),
)
const protectedLayoutParentRoute = createRoute({
getParentRoute: () => protectedLayoutRoute,
path: '/parent',
}).lazy(() =>
import('./lazy/normal').then((f) => f.Route('/_protected/_layout/parent')),
)
const protectedLayoutParentChildRoute = createRoute({
getParentRoute: () => protectedLayoutParentRoute,
path: '/child',
}).lazy(() =>
import('./lazy/normal').then((f) =>
f.Route('/_protected/_layout/parent/child'),
),
)
const protectedFileBasedLayoutParentChildRoute = createRoute({
getParentRoute: () => protectedFileBasedLayoutParentRoute,
path: '/child',
}).lazy(() =>
import('./lazy/normal').then((f) =>
f.FileRoute('/_protected/_fileBasedLayout/fileBasedParent/child'),
),
)

const routeTree = rootRoute.addChildren([
indexRoute,
Expand All @@ -85,6 +133,18 @@ function createTestRouter(initialHistory?: RouterHistory) {
pathSegmentLayoutSplatIndexRoute,
pathSegmentLayoutSplatSplatRoute,
]),
protectedRoute.addChildren([
protectedLayoutRoute.addChildren([
protectedLayoutParentRoute.addChildren([
protectedLayoutParentChildRoute,
]),
]),
protectedFileBasedLayoutRoute.addChildren([
protectedFileBasedLayoutParentRoute.addChildren([
protectedFileBasedLayoutParentChildRoute,
]),
]),
]),
])

const router = createRouter({ routeTree, history })
Expand Down Expand Up @@ -580,3 +640,77 @@ describe('router matches URLs to route definitions', () => {
])
})
})

describe('route ids should be consistent after rebuilding the route tree', () => {
it('should have the same route ids after rebuilding the route tree', async () => {
const { router } = createTestRouter(
createMemoryHistory({ initialEntries: ['/'] }),
)

const originalRouteIds = Object.keys(router.routesById)

await act(() =>
router.navigate({
to: '/parent/child',
}),
)

await act(() =>
router.navigate({
to: '/filBasedParent/child',
}),
)

router.buildRouteTree()

const rebuiltRouteIds = Object.keys(router.routesById)

originalRouteIds.forEach((id) => {
expect(rebuiltRouteIds).toContain(id)
})

rebuiltRouteIds.forEach((id) => {
expect(originalRouteIds).toContain(id)
})
})
})

describe('route id uniqueness', () => {
it('flatRoute should not have routes with duplicated route ids', () => {
const { router } = createTestRouter(
createMemoryHistory({ initialEntries: ['/'] }),
)
const routeIdSet = new Set<string>()

router.flatRoutes.forEach((route) => {
expect(routeIdSet.has(route.id)).toBe(false)
routeIdSet.add(route.id)
})
})

it('routesById should not have routes duplicated route ids', () => {
const { router } = createTestRouter(
createMemoryHistory({ initialEntries: ['/'] }),
)

const routeIdSet = new Set<string>()

Object.values(router.routesById).forEach((route) => {
expect(routeIdSet.has(route.id)).toBe(false)
routeIdSet.add(route.id)
})
})

it('routesByPath should not have routes duplicated route ids', () => {
const { router } = createTestRouter(
createMemoryHistory({ initialEntries: ['/'] }),
)

const routeIdSet = new Set<string>()

Object.values(router.routesByPath).forEach((route) => {
expect(routeIdSet.has(route.id)).toBe(false)
routeIdSet.add(route.id)
})
})
})