fix: improved hybrid hook support #41611
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wyattjoh
force-pushed
the
wyattjoh/improved-hybrid-hook-support
branch
from
4e6c49e
to
6a091a5
Compare
wyattjoh
requested review from
timneutkens,
ijjk,
shuding and
huozhi
as code owners
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. |
timneutkens
reviewed
Oct 22, 2022
| 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.
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.
That makes sense! I can look into making these changes.
timneutkens
reviewed
Oct 22, 2022
| @@ -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.
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.
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.
wyattjoh
force-pushed
the
wyattjoh/improved-hybrid-hook-support
branch
from
6a091a5
to
8e1b754
Compare
|
Closed in favor of #41767 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #numbercontributing.mdFeature
fixes #numbercontributing.mdDocumentation / Examples
pnpm lint