-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: pass routeId into context/beforeLoad #6579
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
Conversation
📝 WalkthroughWalkthroughThreads a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Sheraff
left a comment
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.
LGTM
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.
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/react-router/tests/routeContext.test.tsx (1)
13-23:⚠️ Potential issue | 🟡 MinorUse
rootRouteId(or drop the import) to satisfy lint.ESLint reports the import as unused. The hardcoded
'__root__'string at line 727 should be replaced withrootRouteIdto fix this.Suggested fix
- expect(mock).toHaveBeenCalledWith('__root__') + expect(mock).toHaveBeenCalledWith(rootRouteId)Also applies to: 710-728
|
View your CI Pipeline Execution ↗ for commit 4a833e9
☁️ Nx Cloud last updated this comment at |
3c6f2aa to
9aefe0d
Compare
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.
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/src/route.ts (1)
860-874:⚠️ Potential issue | 🟡 MinorRouteContextFn generic order misaligns with RouteContextOptions signature.
RouteContextFnpassesTSearchValidatorand omitsTLoaderDepswhen constructingRouteContextOptions, causing parameter positions to shift. The correct order should beTParentRoute, TParams, TRouterContext, TLoaderDeps, TRouteIdto match theRouteContextOptionsinterface definition.🔧 Suggested fix
export type RouteContextFn< in out TParentRoute extends AnyRoute, in out TSearchValidator, in out TParams, in out TRouterContext, + in out TLoaderDeps, in out TRouteId, > = ( ctx: RouteContextOptions< TParentRoute, - TSearchValidator, TParams, TRouterContext, + TLoaderDeps, TRouteId >, ) => any
Summary by CodeRabbit
New Features
Tests