-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
base: main
Are you sure you want to change the base?
Conversation
can you please add a test for this? |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit d047bb9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
No problem, I'll do it later today. Actually I found this fix won't work for lazy pathless routes, since they don't have path, will always use |
@schiller-manuel |
@@ -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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
#2400