Ensure interpolating dynamic href values works correctly (#16774)

This corrects/makes sure interpolating dynamic route values for `href` works correctly. This provides an alternative approach to building the `href` value with `next/link` so that you don't need to worry about encoding the params manually. 

Closes: #13473
Closes: #14959
Closes: #16771

packages/next/client/link.tsx

@@ -257,10 +257,12 @@ function Link(props: React.PropsWithChildren<LinkProps>) {
   const pathname = (router && router.pathname) || '/'
 
   const { href, as } = React.useMemo(() => {
-    const resolvedHref = resolveHref(pathname, props.href)
+    const [resolvedHref, resolvedAs] = resolveHref(pathname, props.href, true)
     return {
       href: resolvedHref,
-      as: props.as ? resolveHref(pathname, props.as) : resolvedHref,
+      as: props.as
+        ? resolveHref(pathname, props.as)
+        : resolvedAs || resolvedHref,
     }
   }, [pathname, props.href, props.as])
 

packages/next/client/page-loader.ts

@@ -3,14 +3,16 @@ import type { ClientSsgManifest } from '../build'
 import type { ClientBuildManifest } from '../build/webpack/plugins/build-manifest-plugin'
 import mitt from '../next-server/lib/mitt'
 import type { MittEmitter } from '../next-server/lib/mitt'
-import { addBasePath, markLoadingError } from '../next-server/lib/router/router'
-import escapePathDelimiters from '../next-server/lib/router/utils/escape-path-delimiters'
+import {
+  addBasePath,
+  markLoadingError,
+  interpolateAs,
+} from '../next-server/lib/router/router'
+
 import getAssetPathFromRoute from '../next-server/lib/router/utils/get-asset-path-from-route'
 import { isDynamicRoute } from '../next-server/lib/router/utils/is-dynamic'
 import { parseRelativeUrl } from '../next-server/lib/router/utils/parse-relative-url'
 import { searchParamsToUrlQuery } from '../next-server/lib/router/utils/querystring'
-import { getRouteMatcher } from '../next-server/lib/router/utils/route-matcher'
-import { getRouteRegex } from '../next-server/lib/router/utils/route-regex'
 
 export const looseToArray = <T extends {}>(input: any): T[] =>
   [].slice.call(input)
@@ -216,51 +218,10 @@ export default class PageLoader {
       )
     }
 
-    let isDynamic: boolean = isDynamicRoute(route),
-      interpolatedRoute: string | undefined
-    if (isDynamic) {
-      const dynamicRegex = getRouteRegex(route)
-      const dynamicGroups = dynamicRegex.groups
-      const dynamicMatches =
-        // Try to match the dynamic route against the asPath
-        getRouteMatcher(dynamicRegex)(asPathname) ||
-        // Fall back to reading the values from the href
-        // TODO: should this take priority; also need to change in the router.
-        query
-
-      interpolatedRoute = route
-      if (
-        !Object.keys(dynamicGroups).every((param) => {
-          let value = dynamicMatches[param] || ''
-          const { repeat, optional } = dynamicGroups[param]
-
-          // support single-level catch-all
-          // TODO: more robust handling for user-error (passing `/`)
-          let replaced = `[${repeat ? '...' : ''}${param}]`
-          if (optional) {
-            replaced = `${!value ? '/' : ''}[${replaced}]`
-          }
-          if (repeat && !Array.isArray(value)) value = [value]
-
-          return (
-            (optional || param in dynamicMatches) &&
-            // Interpolate group into data URL if present
-            (interpolatedRoute =
-              interpolatedRoute!.replace(
-                replaced,
-                repeat
-                  ? (value as string[]).map(escapePathDelimiters).join('/')
-                  : escapePathDelimiters(value as string)
-              ) || '/')
-          )
-        })
-      ) {
-        interpolatedRoute = '' // did not satisfy all requirements
-
-        // n.b. We ignore this error because we handle warning for this case in
-        // development in the `<Link>` component directly.
-      }
-    }
+    const isDynamic: boolean = isDynamicRoute(route)
+    const interpolatedRoute = isDynamic
+      ? interpolateAs(hrefPathname, asPathname, query)
+      : ''
 
     return isDynamic
       ? interpolatedRoute && getHrefForSlug(interpolatedRoute)

packages/next/next-server/lib/router/router.ts

@@ -25,6 +25,7 @@ import { searchParamsToUrlQuery } from './utils/querystring'
 import resolveRewrites from './utils/resolve-rewrites'
 import { getRouteMatcher } from './utils/route-matcher'
 import { getRouteRegex } from './utils/route-regex'
+import escapePathDelimiters from './utils/escape-path-delimiters'
 
 interface TransitionOptions {
   shallow?: boolean
@@ -80,24 +81,98 @@ export function isLocalURL(url: string): boolean {
 
 type Url = UrlObject | string
 
+export function interpolateAs(
+  route: string,
+  asPathname: string,
+  query: ParsedUrlQuery
+) {
+  let interpolatedRoute = ''
+
+  const dynamicRegex = getRouteRegex(route)
+  const dynamicGroups = dynamicRegex.groups
+  const dynamicMatches =
+    // Try to match the dynamic route against the asPath
+    (asPathname !== route ? getRouteMatcher(dynamicRegex)(asPathname) : '') ||
+    // Fall back to reading the values from the href
+    // TODO: should this take priority; also need to change in the router.
+    query
+
+  interpolatedRoute = route
+  if (
+    !Object.keys(dynamicGroups).every((param) => {
+      let value = dynamicMatches[param] || ''
+      const { repeat, optional } = dynamicGroups[param]
+
+      // support single-level catch-all
+      // TODO: more robust handling for user-error (passing `/`)
+      let replaced = `[${repeat ? '...' : ''}${param}]`
+      if (optional) {
+        replaced = `${!value ? '/' : ''}[${replaced}]`
+      }
+      if (repeat && !Array.isArray(value)) value = [value]
+
+      return (
+        (optional || param in dynamicMatches) &&
+        // Interpolate group into data URL if present
+        (interpolatedRoute =
+          interpolatedRoute!.replace(
+            replaced,
+            repeat
+              ? (value as string[]).map(escapePathDelimiters).join('/')
+              : escapePathDelimiters(value as string)
+          ) || '/')
+      )
+    })
+  ) {
+    interpolatedRoute = '' // did not satisfy all requirements
+
+    // n.b. We ignore this error because we handle warning for this case in
+    // development in the `<Link>` component directly.
+  }
+  return interpolatedRoute
+}
+
 /**
  * Resolves a given hyperlink with a certain router state (basePath not included).
  * Preserves absolute urls.
  */
-export function resolveHref(currentPath: string, href: Url): string {
+export function resolveHref(
+  currentPath: string,
+  href: Url,
+  resolveAs?: boolean
+): string {
   // we use a dummy base url for relative urls
   const base = new URL(currentPath, 'http://n')
   const urlAsString =
     typeof href === 'string' ? href : formatWithValidation(href)
   try {
     const finalUrl = new URL(urlAsString, base)
     finalUrl.pathname = normalizePathTrailingSlash(finalUrl.pathname)
+    let interpolatedAs = ''
+
+    if (
+      isDynamicRoute(finalUrl.pathname) &&
+      finalUrl.searchParams &&
+      resolveAs
+    ) {
+      const query = searchParamsToUrlQuery(finalUrl.searchParams)
+
+      interpolatedAs = interpolateAs(
+        finalUrl.pathname,
+        finalUrl.pathname,
+        query
+      )
+    }
+
     // if the origin didn't change, it means we received a relative href
-    return finalUrl.origin === base.origin
-      ? finalUrl.href.slice(finalUrl.origin.length)
-      : finalUrl.href
+    const resolvedHref =
+      finalUrl.origin === base.origin
+        ? finalUrl.href.slice(finalUrl.origin.length)
+        : finalUrl.href
+
+    return (resolveAs ? [resolvedHref, interpolatedAs] : resolvedHref) as string
   } catch (_) {
-    return urlAsString
+    return (resolveAs ? [urlAsString] : urlAsString) as string
   }
 }
 
@@ -558,6 +633,8 @@ export default class Router implements BaseRouter {
               `Read more: https://err.sh/vercel/next.js/incompatible-href-as`
           )
         }
+      } else if (route === asPathname) {
+        as = interpolateAs(route, asPathname, query)
       } else {
         // Merge params into `query`, overwriting any specified in search
         Object.assign(query, routeMatch)

test/integration/dynamic-routing/pages/index.js

@@ -10,7 +10,16 @@ const Page = () => {
       </Link>
       <br />
       <Link href="/post-1">
-        <a id="view-post-1-no-as">View post 1</a>
+        <a id="view-post-1-no-as">View post 1 (no as)</a>
+      </Link>
+      <br />
+      <Link
+        href={{
+          pathname: '/[name]',
+          query: { name: 'post-1' },
+        }}
+      >
+        <a id="view-post-1-interpolated">View post 1 (interpolated)</a>
       </Link>
       <br />
       <Link href="/[name]/comments" as="/post-1/comments">
@@ -22,7 +31,18 @@ const Page = () => {
       </Link>
       <br />
       <Link href="/post-1/comment-1">
-        <a id="view-post-1-comment-1-no-as">View comment 1 on post 1</a>
+        <a id="view-post-1-comment-1-no-as">View comment 1 on post 1 (no as)</a>
+      </Link>
+      <br />
+      <Link
+        href={{
+          pathname: '/[name]/[comment]',
+          query: { name: 'post-1', comment: 'comment-1' },
+        }}
+      >
+        <a id="view-post-1-comment-1-interpolated">
+          View comment 1 on post 1 (interpolated)
+        </a>
       </Link>
       <br />
       <Link href="/added-later/first">
@@ -40,42 +60,74 @@ const Page = () => {
       <Link href="/on-mount/[post]" as="/on-mount/test-w-hash#item-400">
         <a id="view-dynamic-with-hash">View test with hash</a>
       </Link>
+      <br />
       <Link href="/p1/p2/all-ssr/[...rest]" as="/p1/p2/all-ssr/hello">
         <a id="catch-all-single">Catch-all route (single)</a>
       </Link>
+      <br />
       <Link href="/p1/p2/all-ssr/[...rest]" as="/p1/p2/all-ssr/hello1/hello2">
         <a id="catch-all-multi">Catch-all route (multi)</a>
       </Link>
+      <br />
       <Link
         href="/p1/p2/all-ssr/[...rest]"
         as="/p1/p2/all-ssr/hello1%2F/he%2Fllo2"
       >
         <a id="catch-all-enc">Catch-all route (encoded)</a>
       </Link>
+      <br />
       <Link href="/p1/p2/all-ssr/[...rest]" as="/p1/p2/all-ssr/:42">
         <a id="catch-all-colonnumber">Catch-all route :42</a>
       </Link>
+      <br />
       <Link href="/p1/p2/all-ssg/[...rest]" as="/p1/p2/all-ssg/hello">
         <a id="ssg-catch-all-single">Catch-all route (single)</a>
       </Link>
+      <br />
+      <Link
+        href={{
+          pathname: '/p1/p2/all-ssg/[...rest]',
+          query: { rest: ['hello'] },
+        }}
+      >
+        <a id="ssg-catch-all-single-interpolated">
+          Catch-all route (single interpolated)
+        </a>
+      </Link>
+      <br />
       <Link href="/p1/p2/all-ssg/[...rest]" as="/p1/p2/all-ssg/hello1/hello2">
         <a id="ssg-catch-all-multi">Catch-all route (multi)</a>
       </Link>
+      <br />
       <Link href="/p1/p2/all-ssg/hello1/hello2">
         <a id="ssg-catch-all-multi-no-as">Catch-all route (multi)</a>
       </Link>
+      <br />
+      <Link
+        href={{
+          pathname: '/p1/p2/all-ssg/[...rest]',
+          query: { rest: ['hello1', 'hello2'] },
+        }}
+      >
+        <a id="ssg-catch-all-multi-interpolated">
+          Catch-all route (multi interpolated)
+        </a>
+      </Link>
+      <br />
       <Link
         href="/p1/p2/nested-all-ssg/[...rest]"
         as="/p1/p2/nested-all-ssg/hello"
       >
         <a id="nested-ssg-catch-all-single">Nested Catch-all route (single)</a>
       </Link>
+      <br />
       <Link
         href="/p1/p2/nested-all-ssg/[...rest]"
         as="/p1/p2/nested-all-ssg/hello1/hello2"
       >
         <a id="nested-ssg-catch-all-multi">Nested Catch-all route (multi)</a>
       </Link>
+      <br />
       <p id="query">{JSON.stringify(Object.keys(useRouter().query))}</p>
     </div>
   )