Fix trailing slash for canonical url (#62109)

### What We should respect the `trailingSlash` config for metadata canonical url, this PR is adding the handling for strip or keep the trailing slash for canonical url. Passing down trailingSlash config to metadata resolving to decide how we handle it. ### Why The tricky one was `/` pathname, when visiting the origin directly, that it will always have at least `/` in the URL instance. But for the default `origin`, it shouldn't show the `/` if the `trailingSlash` config is `false`. Also it should show trailing slash for all pathnames if that config is enabled. BTW there's a `__NEXT_TRAILING_SLASH` env but since we're using the fixed nextjs runtime module, so this can't be dynamically replaced in the metadata resolving modules. So we didn't use it Fixes #54070 Closes NEXT-2424
vercel · Feb 15, 2024 · dc71a57 · dc71a57
1 parent e61497f
commit dc71a57
Show file tree

Hide file tree

Showing 15 changed files with 79 additions and 25 deletions.
diff --git a/packages/next/src/export/index.ts b/packages/next/src/export/index.ts
@@ -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,

diff --git a/packages/next/src/lib/metadata/metadata.tsx b/packages/next/src/lib/metadata/metadata.tsx
@@ -39,6 +39,7 @@ import { isNotFoundError } from '../../client/components/not-found'
 export function createMetadataComponents({
   tree,
   pathname,
+  trailingSlash,
   query,
   getDynamicParamFromSegment,
   appUsingSizeAdjustment,
@@ -47,6 +48,7 @@ export function createMetadataComponents({
 }: {
   tree: LoaderTree
   pathname: string
+  trailingSlash: boolean
   query: ParsedUrlQuery
   getDynamicParamFromSegment: GetDynamicParamFromSegment
   appUsingSizeAdjustment: boolean
@@ -57,6 +59,7 @@ export function createMetadataComponents({
 }): [React.ComponentType, React.ComponentType] {
   const metadataContext = {
     pathname,
+    trailingSlash,
   }
 
   let resolve: (value: Error | undefined) => void | undefined

diff --git a/packages/next/src/lib/metadata/resolve-metadata.test.ts b/packages/next/src/lib/metadata/resolve-metadata.test.ts
@@ -16,6 +16,7 @@ function accumulateMetadata(metadataItems: MetadataItems) {
   ])
   return originAccumulateMetadata(fullMetadataItems, {
     pathname: '/test',
+    trailingSlash: false,
   })
 }
 

diff --git a/packages/next/src/lib/metadata/resolvers/resolve-basics.ts b/packages/next/src/lib/metadata/resolvers/resolve-basics.ts
@@ -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,
@@ -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')
@@ -51,7 +57,7 @@ function resolveUrlValuesOfObject(
     | null
     | undefined,
   metadataBase: ResolvedMetadata['metadataBase'],
-  pathname: string
+  metadataContext: MetadataContext
 ): null | Record<string, AlternateLinkDescriptor[]> {
   if (!obj) return null
 
@@ -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,
@@ -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
 
@@ -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 = {
@@ -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,
   }
 }
diff --git a/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts b/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts
@@ -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) {
@@ -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

diff --git a/packages/next/src/lib/metadata/resolvers/resolve-url.ts b/packages/next/src/lib/metadata/resolvers/resolve-url.ts
@@ -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
@@ -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 {

diff --git a/packages/next/src/lib/metadata/types/resolvers.ts b/packages/next/src/lib/metadata/types/resolvers.ts
@@ -15,4 +15,5 @@ export type FieldResolverExtraArgs<
 
 export type MetadataContext = {
   pathname: string
+  trailingSlash: boolean
 }
diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx
@@ -287,6 +287,7 @@ async function generateFlight(
     const [MetadataTree, MetadataOutlet] = createMetadataComponents({
       tree: loaderTree,
       pathname: urlPathname,
+      trailingSlash: ctx.renderOpts.trailingSlash,
       query,
       getDynamicParamFromSegment,
       appUsingSizeAdjustment,
@@ -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,
@@ -506,6 +508,7 @@ async function ReactServerError({
   const [MetadataTree] = createMetadataComponents({
     tree,
     pathname: urlPathname,
+    trailingSlash: ctx.renderOpts.trailingSlash,
     errorType,
     query,
     getDynamicParamFromSegment,

diff --git a/packages/next/src/server/app-render/types.ts b/packages/next/src/server/app-render/types.ts
@@ -111,6 +111,7 @@ export interface RenderOptsPartial {
   dev?: boolean
   buildId: string
   basePath: string
+  trailingSlash: boolean
   clientReferenceManifest?: ClientReferenceManifest
   supportsDynamicHTML: boolean
   runtime?: ServerRuntime

diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts
@@ -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,

diff --git a/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts b/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
@@ -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 () => {

diff --git a/test/e2e/app-dir/metadata-dynamic-routes/next.config.js b/test/e2e/app-dir/metadata-dynamic-routes/next.config.js
@@ -1,3 +1,4 @@
+/** @type {import('next').NextConfig} */
 module.exports = {}
 
 // For development: analyze the bundled chunks for stats app

diff --git a/test/e2e/app-dir/metadata/metadata.test.ts b/test/e2e/app-dir/metadata/metadata.test.ts
@@ -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',

diff --git a/test/e2e/app-dir/trailingslash/app/layout.js b/test/e2e/app-dir/trailingslash/app/layout.js
@@ -8,3 +8,10 @@ export default function Root({ children }) {
     </html>
   )
 }
+
+export const metadata = {
+  metadataBase: new URL('http://trailingslash.com'),
+  alternates: {
+    canonical: './',
+  },
+}
diff --git a/test/e2e/app-dir/trailingslash/trailingslash.test.ts b/test/e2e/app-dir/trailingslash/trailingslash.test.ts
@@ -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')