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

Fix trailing slash for canonical url #62109

Merged
merged 4 commits into from
Feb 15, 2024
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
1 change: 1 addition & 0 deletions packages/next/src/export/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@ export async function exportAppImpl(
distDir,
dev: false,
basePath: nextConfig.basePath,
trailingSlash: nextConfig.trailingSlash,
canonicalBase: nextConfig.amp?.canonicalBase || '',
ampSkipValidation: nextConfig.experimental.amp?.skipValidation || false,
ampOptimizerConfig: nextConfig.experimental.amp?.optimizer || undefined,
Expand Down
3 changes: 3 additions & 0 deletions packages/next/src/lib/metadata/metadata.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { isNotFoundError } from '../../client/components/not-found'
export function createMetadataComponents({
tree,
pathname,
trailingSlash,
query,
getDynamicParamFromSegment,
appUsingSizeAdjustment,
Expand All @@ -47,6 +48,7 @@ export function createMetadataComponents({
}: {
tree: LoaderTree
pathname: string
trailingSlash: boolean
query: ParsedUrlQuery
getDynamicParamFromSegment: GetDynamicParamFromSegment
appUsingSizeAdjustment: boolean
Expand All @@ -57,6 +59,7 @@ export function createMetadataComponents({
}): [React.ComponentType, React.ComponentType] {
const metadataContext = {
pathname,
trailingSlash,
}

let resolve: (value: Error | undefined) => void | undefined
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/lib/metadata/resolve-metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ function accumulateMetadata(metadataItems: MetadataItems) {
])
return originAccumulateMetadata(fullMetadataItems, {
pathname: '/test',
trailingSlash: false,
})
}

Expand Down
42 changes: 24 additions & 18 deletions packages/next/src/lib/metadata/resolvers/resolve-basics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import type {
AlternateLinkDescriptor,
ResolvedAlternateURLs,
} from '../types/alternative-urls-types'
import type { Metadata, ResolvedMetadata } from '../types/metadata-interface'
import type {
Metadata,
ResolvedMetadata,
Viewport,
} from '../types/metadata-interface'
import type { ResolvedVerification } from '../types/metadata-types'
import type {
FieldResolver,
Expand All @@ -15,19 +19,21 @@ import { resolveAbsoluteUrlWithPathname } from './resolve-url'
function resolveAlternateUrl(
url: string | URL,
metadataBase: URL | null,
pathname: string
metadataContext: MetadataContext
) {
// If alter native url is an URL instance,
// we treat it as a URL base and resolve with current pathname
if (url instanceof URL) {
url = new URL(pathname, url)
url = new URL(metadataContext.pathname, url)
}
return resolveAbsoluteUrlWithPathname(url, metadataBase, pathname)
return resolveAbsoluteUrlWithPathname(url, metadataBase, metadataContext)
}

export const resolveThemeColor: FieldResolver<'themeColor'> = (themeColor) => {
export const resolveThemeColor: FieldResolver<'themeColor', Viewport> = (
themeColor
) => {
if (!themeColor) return null
const themeColorDescriptors: ResolvedMetadata['themeColor'] = []
const themeColorDescriptors: Viewport['themeColor'] = []

resolveAsArrayOrUndefined(themeColor)?.forEach((descriptor) => {
if (typeof descriptor === 'string')
Expand All @@ -51,7 +57,7 @@ function resolveUrlValuesOfObject(
| null
| undefined,
metadataBase: ResolvedMetadata['metadataBase'],
pathname: string
metadataContext: MetadataContext
): null | Record<string, AlternateLinkDescriptor[]> {
if (!obj) return null

Expand All @@ -60,13 +66,13 @@ function resolveUrlValuesOfObject(
if (typeof value === 'string' || value instanceof URL) {
result[key] = [
{
url: resolveAlternateUrl(value, metadataBase, pathname),
url: resolveAlternateUrl(value, metadataBase, metadataContext),
},
]
} else {
result[key] = []
value?.forEach((item, index) => {
const url = resolveAlternateUrl(item.url, metadataBase, pathname)
const url = resolveAlternateUrl(item.url, metadataBase, metadataContext)
result[key][index] = {
url,
title: item.title,
Expand All @@ -80,7 +86,7 @@ function resolveUrlValuesOfObject(
function resolveCanonicalUrl(
urlOrDescriptor: string | URL | null | AlternateLinkDescriptor | undefined,
metadataBase: URL | null,
pathname: string
metadataContext: MetadataContext
): null | AlternateLinkDescriptor {
if (!urlOrDescriptor) return null

Expand All @@ -91,35 +97,35 @@ function resolveCanonicalUrl(

// Return string url because structureClone can't handle URL instance
return {
url: resolveAlternateUrl(url, metadataBase, pathname),
url: resolveAlternateUrl(url, metadataBase, metadataContext),
}
}

export const resolveAlternates: FieldResolverExtraArgs<
'alternates',
[ResolvedMetadata['metadataBase'], MetadataContext]
> = (alternates, metadataBase, { pathname }) => {
> = (alternates, metadataBase, context) => {
if (!alternates) return null

const canonical = resolveCanonicalUrl(
alternates.canonical,
metadataBase,
pathname
context
)
const languages = resolveUrlValuesOfObject(
alternates.languages,
metadataBase,
pathname
context
)
const media = resolveUrlValuesOfObject(
alternates.media,
metadataBase,
pathname
context
)
const types = resolveUrlValuesOfObject(
alternates.types,
metadataBase,
pathname
context
)

const result: ResolvedAlternateURLs = {
Expand Down Expand Up @@ -236,12 +242,12 @@ export const resolveAppLinks: FieldResolver<'appLinks'> = (appLinks) => {
export const resolveItunes: FieldResolverExtraArgs<
'itunes',
[ResolvedMetadata['metadataBase'], MetadataContext]
> = (itunes, metadataBase, { pathname }) => {
> = (itunes, metadataBase, context) => {
if (!itunes) return null
return {
appId: itunes.appId,
appArgument: itunes.appArgument
? resolveAlternateUrl(itunes.appArgument, metadataBase, pathname)
? resolveAlternateUrl(itunes.appArgument, metadataBase, context)
: undefined,
}
}
8 changes: 6 additions & 2 deletions packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ function getFieldsByOgType(ogType: OpenGraphType | undefined) {
export const resolveOpenGraph: FieldResolverExtraArgs<
'openGraph',
[ResolvedMetadata['metadataBase'], MetadataContext, string | null]
> = (openGraph, metadataBase, { pathname }, titleTemplate) => {
> = (openGraph, metadataBase, metadataContext, titleTemplate) => {
if (!openGraph) return null

function resolveProps(target: ResolvedOpenGraph, og: OpenGraph) {
Expand Down Expand Up @@ -126,7 +126,11 @@ export const resolveOpenGraph: FieldResolverExtraArgs<
resolveProps(resolved, openGraph)

resolved.url = openGraph.url
? resolveAbsoluteUrlWithPathname(openGraph.url, metadataBase, pathname)
? resolveAbsoluteUrlWithPathname(
openGraph.url,
metadataBase,
metadataContext
)
: null

return resolved
Expand Down
18 changes: 15 additions & 3 deletions packages/next/src/lib/metadata/resolvers/resolve-url.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import path from '../../../shared/lib/isomorphic/path'
import * as Log from '../../../build/output/log'
import type { MetadataContext } from '../types/resolvers'

function isStringOrURL(icon: any): icon is string | URL {
return typeof icon === 'string' || icon instanceof URL
Expand Down Expand Up @@ -83,12 +84,23 @@ function resolveRelativeUrl(url: string | URL, pathname: string): string | URL {
function resolveAbsoluteUrlWithPathname(
url: string | URL,
metadataBase: URL | null,
pathname: string
) {
{ trailingSlash, pathname }: MetadataContext
): string {
url = resolveRelativeUrl(url, pathname)

// Get canonicalUrl without trailing slash
let canonicalUrl = ''
const result = metadataBase ? resolveUrl(url, metadataBase) : url
return result.toString()
if (typeof result === 'string') {
canonicalUrl = result
} else {
canonicalUrl = result.pathname === '/' ? result.origin : result.href
}

// Add trailing slash if it's enabled
return trailingSlash && !canonicalUrl.endsWith('/')
? `${canonicalUrl}/`
: canonicalUrl
}

export {
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/lib/metadata/types/resolvers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ export type FieldResolverExtraArgs<

export type MetadataContext = {
pathname: string
trailingSlash: boolean
}
3 changes: 3 additions & 0 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ async function generateFlight(
const [MetadataTree, MetadataOutlet] = createMetadataComponents({
tree: loaderTree,
pathname: urlPathname,
trailingSlash: ctx.renderOpts.trailingSlash,
query,
getDynamicParamFromSegment,
appUsingSizeAdjustment,
Expand Down Expand Up @@ -420,6 +421,7 @@ async function ReactServerApp({ tree, ctx, asNotFound }: ReactServerAppProps) {
tree,
errorType: asNotFound ? 'not-found' : undefined,
pathname: urlPathname,
trailingSlash: ctx.renderOpts.trailingSlash,
query,
getDynamicParamFromSegment: getDynamicParamFromSegment,
appUsingSizeAdjustment: appUsingSizeAdjustment,
Expand Down Expand Up @@ -506,6 +508,7 @@ async function ReactServerError({
const [MetadataTree] = createMetadataComponents({
tree,
pathname: urlPathname,
trailingSlash: ctx.renderOpts.trailingSlash,
errorType,
query,
getDynamicParamFromSegment,
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/app-render/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export interface RenderOptsPartial {
dev?: boolean
buildId: string
basePath: string
trailingSlash: boolean
clientReferenceManifest?: ClientReferenceManifest
supportsDynamicHTML: boolean
runtime?: ServerRuntime
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {

this.renderOpts = {
supportsDynamicHTML: true,
trailingSlash: this.nextConfig.trailingSlash,
deploymentId: this.nextConfig.experimental.deploymentId,
strictNextHead: !!this.nextConfig.experimental.strictNextHead,
poweredByHeader: this.nextConfig.poweredByHeader,
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ createNextDescribe(
it('should pick configured metadataBase instead of deployment url for canonical url', async () => {
const $ = await next.render$('/')
const canonicalUrl = $('link[rel="canonical"]').attr('href')
expect(canonicalUrl).toBe('https://mydomain.com/')
expect(canonicalUrl).toBe('https://mydomain.com')
})

it('should inject dynamic metadata properly to head', async () => {
Expand Down
1 change: 1 addition & 0 deletions test/e2e/app-dir/metadata-dynamic-routes/next.config.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/** @type {import('next').NextConfig} */
module.exports = {}

// For development: analyze the bundled chunks for stats app
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/app-dir/metadata/metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ createNextDescribe(
await matchMultiDom('meta', 'property', 'content', {
'og:title': 'My custom title',
'og:description': 'My custom description',
'og:url': 'https://example.com/',
'og:url': 'https://example.com',
'og:site_name': 'My custom site name',
'og:locale': 'en-US',
'og:type': 'website',
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/app-dir/trailingslash/app/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,10 @@ export default function Root({ children }) {
</html>
)
}

export const metadata = {
metadataBase: new URL('http://trailingslash.com'),
alternates: {
canonical: './',
},
}
13 changes: 13 additions & 0 deletions test/e2e/app-dir/trailingslash/trailingslash.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,22 @@ createNextDescribe(

it('should render link with trailing slash', async () => {
const $ = await next.render$('/')

expect($('#to-a-trailing-slash').attr('href')).toBe('/a/')
})

it('should contain trailing slash to canonical url', async () => {
const $ = await next.render$('/')
expect($(`link[rel="canonical"]`).attr('href')).toBe(
'http://trailingslash.com/'
)

const $a = await next.render$('/a')
expect($a(`link[rel="canonical"]`).attr('href')).toBe(
'http://trailingslash.com/a/'
)
})

it('should redirect route when requesting it directly by browser', async () => {
const browser = await next.browser('/a')
expect(await browser.waitForElementByCss('#a-page').text()).toBe('A page')
Expand Down