-
Notifications
You must be signed in to change notification settings - Fork 402
fix(nextjs): Shallow routing for in-component navigations #5091
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
fix(nextjs): Shallow routing for in-component navigations #5091
Conversation
🦋 Changeset detectedLatest commit: f953513 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| return useCallback((to: string) => { | ||
| return getClerkNavigationObject(name).fun(to); | ||
| return useCallback<NavigationFunction>((to, metadata) => { | ||
| return getClerkNavigationObject(name).fun(to, metadata); |
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.
mind sharing an instance of where we were passing the second argument metadata to push/replace? I want to look into why that wasn't causing a typescript error. useAwaitableReplace/useAwaitablePush should have only accepted a single argument.
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.
useAwaitableReplace/useAwaitablePush should have only accepted a single argument.
the hooks themselves do not take any arguments, previously they were returning a fuction that would accept a single argument, but in Clerk.navigate we are clearly passing 2 arguments.
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.
it was not giving any typescript errors because the 2nd argument is optional
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.
TypeScript is inferring the type as const useAwaitablePush: () => (to: string) => unknown though, so the returned function should be typed as only accepting a single argument. I guess we're losing that type safety somewhere along the way 🤔
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.
ahhh okay clerk-js accepts that as RouterFn which has the optional metadata argument. Since the previous type was (to: string) => unknown, that satisfies (to: string, metadata?: whatever) => void.
Description
Before (notice the brief flash of the page)
Screen.Recording.2025-02-06.at.6.33.06.PM.mov
After
Screen.Recording.2025-02-06.at.6.31.42.PM.mov
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change