From 9a93e2516a354d9e1ffe8e9db44544e27cbb9d34 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 2 Nov 2022 14:59:39 -0600 Subject: [PATCH 1/2] feat: change `usePathname` to return `string | null` --- packages/next/client/components/navigation.ts | 10 +--- packages/next/client/index.tsx | 15 +++--- packages/next/server/render.tsx | 15 +++--- .../lib/router/{adapters.ts => adapters.tsx} | 48 ++++++++++++++----- 4 files changed, 54 insertions(+), 34 deletions(-) rename packages/next/shared/lib/router/{adapters.ts => adapters.tsx} (55%) diff --git a/packages/next/client/components/navigation.ts b/packages/next/client/components/navigation.ts index 0d5f4dde633feb4..37423db9f080e58 100644 --- a/packages/next/client/components/navigation.ts +++ b/packages/next/client/components/navigation.ts @@ -76,7 +76,6 @@ export function useSearchParams() { throw new Error('invariant expected search params to be mounted') } - // eslint-disable-next-line react-hooks/rules-of-hooks const readonlySearchParams = useMemo(() => { return new ReadonlyURLSearchParams(searchParams) }, [searchParams]) @@ -87,14 +86,9 @@ export function useSearchParams() { /** * Get the current pathname. For example usePathname() on /dashboard?foo=bar would return "/dashboard" */ -export function usePathname(): string { +export function usePathname(): string | null { staticGenerationBailout('usePathname') - const pathname = useContext(PathnameContext) - if (pathname === null) { - throw new Error('invariant expected pathname to be mounted') - } - - return pathname + return useContext(PathnameContext) } // 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. diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index a63a5ca9b05c911..16aeb6ff7cddf9e 100644 --- a/packages/next/client/index.tsx +++ b/packages/next/client/index.tsx @@ -39,13 +39,10 @@ import { hasBasePath } from './has-base-path' import { AppRouterContext } from '../shared/lib/app-router-context' import { adaptForAppRouterInstance, - adaptForPathname, adaptForSearchParams, + PathnameContextProviderAdapter, } from '../shared/lib/router/adapters' -import { - PathnameContext, - SearchParamsContext, -} from '../shared/lib/hooks-client-context' +import { SearchParamsContext } from '../shared/lib/hooks-client-context' /// @@ -316,7 +313,11 @@ function AppContainer({ > - + - + diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx index b379c3d9a8a70fa..2feddd9a60dd0a9 100644 --- a/packages/next/server/render.tsx +++ b/packages/next/server/render.tsx @@ -84,14 +84,11 @@ import stripAnsi from 'next/dist/compiled/strip-ansi' import { stripInternalQueries } from './internal-utils' import { adaptForAppRouterInstance, - adaptForPathname, adaptForSearchParams, + PathnameContextProviderAdapter, } from '../shared/lib/router/adapters' import { AppRouterContext } from '../shared/lib/app-router-context' -import { - PathnameContext, - SearchParamsContext, -} from '../shared/lib/hooks-client-context' +import { SearchParamsContext } from '../shared/lib/hooks-client-context' let tryGetPreviewData: typeof import('./api-utils/node').tryGetPreviewData let warn: typeof import('../build/output/log').warn @@ -621,7 +618,11 @@ export async function renderToHTML( const AppContainer = ({ children }: { children: JSX.Element }) => ( - + - + ) diff --git a/packages/next/shared/lib/router/adapters.ts b/packages/next/shared/lib/router/adapters.tsx similarity index 55% rename from packages/next/shared/lib/router/adapters.ts rename to packages/next/shared/lib/router/adapters.tsx index facd64c2a7effed..38158a47e500de8 100644 --- a/packages/next/shared/lib/router/adapters.ts +++ b/packages/next/shared/lib/router/adapters.tsx @@ -1,6 +1,9 @@ import type { ParsedUrlQuery } from 'node:querystring' -import { AppRouterInstance } from '../app-router-context' -import { NextRouter } from './router' +import React, { useMemo } from 'react' +import type { AppRouterInstance } from '../app-router-context' +import { PathnameContext } from '../hooks-client-context' +import type { NextRouter } from './router' +import { isDynamicRoute } from './utils' /** * adaptForAppRouterInstance implements the AppRouterInstance with a NextRouter. @@ -61,7 +64,9 @@ function transformQuery(query: ParsedUrlQuery): URLSearchParams { * @param router the router that contains the query. * @returns the search params in the URLSearchParams format */ -export function adaptForSearchParams(router: NextRouter): URLSearchParams { +export function adaptForSearchParams( + router: Pick +): URLSearchParams { if (!router.isReady || !router.query) { return new URLSearchParams() } @@ -69,13 +74,32 @@ export function adaptForSearchParams(router: NextRouter): URLSearchParams { return transformQuery(router.query) } -/** - * adaptForPathname adapts the `asPath` parameter from the router to a pathname. - * - * @param asPath the asPath parameter to transform that comes from the router - * @returns pathname part of `asPath` - */ -export function adaptForPathname(asPath: string): string { - const url = new URL(asPath, 'http://f') - return url.pathname +export function PathnameContextProviderAdapter({ + children, + router, + isAutoExport, + isFallback, +}: React.PropsWithChildren<{ + router: Pick + isAutoExport: boolean + isFallback: boolean +}>) { + const value = useMemo(() => { + // If this is a dynamic route with auto export or fallback is true... + if (isDynamicRoute(router.pathname) && (isAutoExport || isFallback)) { + // Return null. This will throw an error when accessed via `usePathname`, + // but it provides the correct API for folks considering the new router + // does not support `isReady`. + return null + } + + const url = new URL(router.asPath, 'http://f') + return url.pathname + }, [router.pathname, router.asPath, isAutoExport, isFallback]) + + return ( + + {children} + + ) } From 31859de23058e2211f751d56a34c02e8ab8bf0d6 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 3 Nov 2022 14:02:34 -0600 Subject: [PATCH 2/2] fix: support fallback + static pages properly --- packages/next/client/index.tsx | 1 - packages/next/server/render.tsx | 1 - packages/next/shared/lib/router/adapters.tsx | 48 +++++++++++++++----- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index 16aeb6ff7cddf9e..a5baeae1648c634 100644 --- a/packages/next/client/index.tsx +++ b/packages/next/client/index.tsx @@ -316,7 +316,6 @@ function AppContainer({ diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx index 2feddd9a60dd0a9..c5b2686a6e2b5f9 100644 --- a/packages/next/server/render.tsx +++ b/packages/next/server/render.tsx @@ -621,7 +621,6 @@ export async function renderToHTML( diff --git a/packages/next/shared/lib/router/adapters.tsx b/packages/next/shared/lib/router/adapters.tsx index 38158a47e500de8..9bf0096abad2be7 100644 --- a/packages/next/shared/lib/router/adapters.tsx +++ b/packages/next/shared/lib/router/adapters.tsx @@ -1,5 +1,5 @@ import type { ParsedUrlQuery } from 'node:querystring' -import React, { useMemo } from 'react' +import React, { useMemo, useRef } from 'react' import type { AppRouterInstance } from '../app-router-context' import { PathnameContext } from '../hooks-client-context' import type { NextRouter } from './router' @@ -77,25 +77,49 @@ export function adaptForSearchParams( export function PathnameContextProviderAdapter({ children, router, - isAutoExport, - isFallback, + ...props }: React.PropsWithChildren<{ - router: Pick + router: Pick isAutoExport: boolean - isFallback: boolean }>) { + const ref = useRef(props.isAutoExport) const value = useMemo(() => { - // If this is a dynamic route with auto export or fallback is true... - if (isDynamicRoute(router.pathname) && (isAutoExport || isFallback)) { - // Return null. This will throw an error when accessed via `usePathname`, - // but it provides the correct API for folks considering the new router - // does not support `isReady`. - return null + // isAutoExport is only ever `true` on the first render from the server, + // so reset it to `false` after we read it for the first time as `true`. If + // we don't use the value, then we don't need it. + const isAutoExport = ref.current + if (isAutoExport) { + ref.current = false } + // When the route is a dynamic route, we need to do more processing to + // determine if we need to stop showing the pathname. + if (isDynamicRoute(router.pathname)) { + // When the router is rendering the fallback page, it can't possibly know + // the path, so return `null` here. Read more about fallback pages over + // at: + // https://nextjs.org/docs/api-reference/data-fetching/get-static-paths#fallback-pages + if (router.isFallback) { + return null + } + + // When `isAutoExport` is true, meaning this is a page page has been + // automatically statically optimized, and the router is not ready, then + // we can't know the pathname yet. Read more about automatic static + // optimization at: + // https://nextjs.org/docs/advanced-features/automatic-static-optimization + if (isAutoExport && !router.isReady) { + return null + } + } + + // The `router.asPath` contains the pathname seen by the browser (including + // any query strings), so it should have that stripped. Read more about the + // `asPath` option over at: + // https://nextjs.org/docs/api-reference/next/router#router-object const url = new URL(router.asPath, 'http://f') return url.pathname - }, [router.pathname, router.asPath, isAutoExport, isFallback]) + }, [router.asPath, router.isFallback, router.isReady, router.pathname]) return (