Skip to content
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

Ensure interpolating dynamic href values works correctly #16774

Merged
merged 5 commits into from Sep 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/next/client/link.tsx
Expand Up @@ -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])

Expand Down
59 changes: 10 additions & 49 deletions packages/next/client/page-loader.ts
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
87 changes: 82 additions & 5 deletions packages/next/next-server/lib/router/router.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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)
Expand Down
56 changes: 54 additions & 2 deletions test/integration/dynamic-routing/pages/index.js
Expand Up @@ -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">
Expand All @@ -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">
Expand All @@ -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>
)
Expand Down