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
Changes from all commits
3840c08
db9f507
24e6de8
8e1b754
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { useContext } from 'react' | ||
import { | ||
AppRouterContext, | ||
AppRouterInstance, | ||
} from '../../shared/lib/app-router-context' | ||
|
||
/** | ||
* useAppRouter will get the AppRouterInstance on the context if it's mounted. | ||
* If it is not mounted, it will throw an error. This method should only be used | ||
* when you expect only to have the app router mounted (not pages router). | ||
* | ||
* @returns the app router instance | ||
*/ | ||
export function useAppRouter(): AppRouterInstance { | ||
const router = useContext(AppRouterContext) | ||
if (!router) { | ||
throw new Error('invariant expected app router to be mounted') | ||
} | ||
|
||
return router | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { AppRouterInstance } from '../../shared/lib/app-router-context' | ||
import { NextRouter } from '../router' | ||
|
||
export const HYBRID_ROUTER_TYPE = Symbol('HYBRID_ROUTER_TYPE') | ||
|
||
type MaskedHybridRouter<T extends string, Router, OtherRouter> = { | ||
// Store the router type on the router via this private symbol. | ||
[HYBRID_ROUTER_TYPE]: T | ||
} & Router & | ||
// Add partial fields of the other router so it's type-compatible with it, but | ||
// it will show those extra fields as undefined. | ||
Partial<Omit<OtherRouter, keyof Router>> | ||
|
||
export type HybridRouter = | ||
| MaskedHybridRouter<'app', AppRouterInstance, NextRouter> | ||
| MaskedHybridRouter<'pages', NextRouter, AppRouterInstance> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,9 @@ import { | |
AppRouterContext, | ||
LayoutRouterContext, | ||
} from '../../shared/lib/app-router-context' | ||
import { RouterContext } from '../../shared/lib/router-context' | ||
import type { ParsedUrlQuery } from 'querystring' | ||
import { HybridRouter, HYBRID_ROUTER_TYPE } from './hybrid-router' | ||
|
||
export { | ||
ServerInsertedHTMLContext, | ||
|
@@ -69,24 +72,80 @@ class ReadonlyURLSearchParams { | |
} | ||
} | ||
|
||
/** | ||
* parsedURLQueryToURLSearchParams converts a parsed url query to a url search | ||
* params object. | ||
* | ||
* @param query parsed url query | ||
* @returns url search params object | ||
*/ | ||
function parsedURLQueryToURLSearchParams( | ||
query: ParsedUrlQuery | ||
): URLSearchParams { | ||
return new URLSearchParams( | ||
Object.keys(query).reduce<[string, string][]>((acc, name) => { | ||
const value = query[name] | ||
if (Array.isArray(value)) { | ||
acc.push(...value.map<[string, string]>((v) => [name, v])) | ||
} else { | ||
acc.push([name, value]) | ||
} | ||
|
||
return acc | ||
}, []) | ||
) | ||
} | ||
|
||
/** | ||
* Get a read-only URLSearchParams object. For example searchParams.get('foo') would return 'bar' when ?foo=bar | ||
* Learn more about URLSearchParams here: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams | ||
*/ | ||
export function useSearchParams() { | ||
const router = useContext(RouterContext) | ||
const searchParams = useContext(SearchParamsContext) | ||
|
||
const readonlySearchParams = useMemo(() => { | ||
return new ReadonlyURLSearchParams(searchParams) | ||
}, [searchParams]) | ||
// To support migration from pages to app, this adds a workaround that'll | ||
// support the pages router here too. | ||
if (router) { | ||
if (!router.isReady) { | ||
return new ReadonlyURLSearchParams(new URLSearchParams()) | ||
} | ||
|
||
return new ReadonlyURLSearchParams( | ||
parsedURLQueryToURLSearchParams(router.query) | ||
) | ||
} | ||
|
||
if (searchParams) { | ||
return new ReadonlyURLSearchParams(searchParams) | ||
} | ||
|
||
throw new Error('invariant at least one router was expected') | ||
}, [router, searchParams]) | ||
|
||
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 { | ||
const router = useContext(RouterContext) | ||
const appRouter = useContext(AppRouterContext) | ||
|
||
return useMemo(() => { | ||
if (router) { | ||
return { [HYBRID_ROUTER_TYPE]: 'pages', ...router } | ||
} | ||
|
||
if (appRouter) { | ||
return { [HYBRID_ROUTER_TYPE]: 'app', ...appRouter } | ||
} | ||
|
||
throw new Error('invariant at least one router was expected') | ||
}, [router, appRouter]) | ||
} | ||
|
||
// TODO-APP: getting all params when client-side navigating is non-trivial as it does not have route matchers so this might have to be a server context instead. | ||
|
@@ -97,8 +156,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 commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should throw in the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
const router = useContext(RouterContext) | ||
const pathname = useContext(PathnameContext) | ||
|
||
if (router) { | ||
if (router.isReady) { | ||
return router.asPath | ||
} | ||
|
||
return null | ||
} | ||
|
||
return pathname | ||
} | ||
|
||
// TODO-APP: define what should be provided through context. | ||
|
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 setAppRouterContext
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.