-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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: improved hybrid hook support #41611
fix: improved hybrid hook support #41611
Conversation
4e6c49e
to
6a091a5
Compare
Failing test suitesCommit: 6a091a5
Expand output● Link without a router › dev mode › should not throw when rendered
● Link without a router › production mode › should not throw when rendered
Read more about building and testing Next.js in contributing.md.
Expand output● next/jest newLinkBehavior › should use new link behavior
Read more about building and testing Next.js in contributing.md.
Expand output● TypeScript Features › should build the app
● TypeScript Features › should build the app with functions in next.config.js
● TypeScript Features › should compile with different types › should compile async getInitialProps for _error
● TypeScript Features › should compile with different types › should compile sync getStaticPaths & getStaticProps
Read more about building and testing Next.js in contributing.md.
Expand output● react 18 streaming SSR with custom next configs › should redirect paths without trailing-slash and render when slash is appended
Read more about building and testing Next.js in contributing.md.
Expand output● Switchable runtime › Switchable runtime (dev) › should sort edge SSR routes correctly
● Switchable runtime › Switchable runtime (dev) › should be able to navigate between edge SSR routes without any errors
Read more about building and testing Next.js in contributing.md.
Expand output● app dir › nested navigation › should navigate to nested pages
Read more about building and testing Next.js in contributing.md. |
return readonlySearchParams | ||
} | ||
|
||
// TODO-APP: Move the other router context over to this one | ||
/** | ||
* Get the router methods. For example router.push('/dashboard') | ||
*/ | ||
export function useRouter(): import('../../shared/lib/app-router-context').AppRouterInstance { | ||
return useContext(AppRouterContext) | ||
export function useRouter(): HybridRouter { |
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.
The way I was thinking about this is that this always returns a AppRouterInstance
, we'd just set AppRouterContext
for the pages router too with it's own AppRouterInstance object that under the hood calls the existing router. Does that make sense? It would keep the type the same regardless and makes migrating easier.
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.
That makes sense! I can look into making these changes.
@@ -97,8 +152,19 @@ export function useRouter(): import('../../shared/lib/app-router-context').AppRo | |||
/** | |||
* Get the current pathname. For example usePathname() on /dashboard?foo=bar would return "/dashboard" | |||
*/ | |||
export function usePathname(): string { | |||
return useContext(PathnameContext) | |||
export function usePathname(): string | null { |
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.
Maybe this should throw in the null
case. I'd prefer it to always be a string
as that's how it's supposed to be in the new 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.
If we change it to throws then during migration, this would cause any component using it from pages to throw too. This sort of defeats the purpose for the hybrid approach.
6a091a5
to
8e1b754
Compare
Closed in favor of #41767 |
These hooks when accessed from different router context's in fact, return
null
. These new router hooks have been adapted to be backwards compatible with the pages router to ease the migration process.Bug
fixes #number
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
pnpm lint