From 378b034b38117c5d34ff4131f21d0b0b4980870c Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 16 Feb 2024 00:29:55 +0100 Subject: [PATCH 1/3] Should not warn metadataBase missing if only absolute urls are present (#61898) * Narrow down the metadata base warnings only when there's any relative urls need to be resolved, if there's only absolute urls present, no need to resolve and we don't warn. * Polish the error message, updated from "metadata.metadataBase is not set ..." to "metadataBase property in metadata export is not set ..." It will be confusing if we're still show metadataBase warning when there's no need to set one, since the social image cards only have absolute urls Closes NEXT-2426 --- .../metadata/resolvers/resolve-opengraph.ts | 87 +++++++++++++----- .../src/lib/metadata/resolvers/resolve-url.ts | 25 +++-- packages/next/src/lib/url.ts | 13 +++ .../app/page.js | 9 -- .../next.config.js | 1 - .../app/absolute-url-og/page.js | 9 ++ .../app/layout.js | 1 - .../og-image-convention}/opengraph-image.png | Bin .../app/og-image-convention/page.js | 3 + .../app/unsupported-metadata/page.js | 0 .../index.test.ts | 25 +++-- 11 files changed, 114 insertions(+), 59 deletions(-) create mode 100644 packages/next/src/lib/url.ts delete mode 100644 test/e2e/app-dir/metadata-missing-metadata-base/app/page.js delete mode 100644 test/e2e/app-dir/metadata-missing-metadata-base/next.config.js create mode 100644 test/e2e/app-dir/metadata-warnings/app/absolute-url-og/page.js rename test/e2e/app-dir/{metadata-missing-metadata-base => metadata-warnings}/app/layout.js (85%) rename test/e2e/app-dir/{metadata-missing-metadata-base/app => metadata-warnings/app/og-image-convention}/opengraph-image.png (100%) create mode 100644 test/e2e/app-dir/metadata-warnings/app/og-image-convention/page.js rename test/e2e/app-dir/{metadata-missing-metadata-base => metadata-warnings}/app/unsupported-metadata/page.js (100%) rename test/e2e/app-dir/{metadata-missing-metadata-base => metadata-warnings}/index.test.ts (65%) diff --git a/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts b/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts index 838cd51d1244b5..9bb526e62d7a3f 100644 --- a/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts +++ b/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts @@ -17,6 +17,11 @@ import { resolveAbsoluteUrlWithPathname, } from './resolve-url' import { resolveTitle } from './resolve-title' +import { isFullStringUrl } from '../../url' +import { warnOnce } from '../../../build/output/log' + +type FlattenArray = T extends (infer U)[] ? U : T +type ResolvedMetadataBase = ResolvedMetadata['metadataBase'] const OgTypeFields = { article: ['authors', 'tags'], @@ -34,41 +39,58 @@ const OgTypeFields = { ], } as const +function resolveAndValidateImage( + item: FlattenArray, + metadataBase: NonNullable, + isMetadataBaseMissing: boolean +) { + if (!item) return undefined + const isItemUrl = isStringOrURL(item) + const inputUrl = isItemUrl ? item : item.url + if (!inputUrl) return undefined + + validateResolvedImageUrl(inputUrl, metadataBase, isMetadataBaseMissing) + + return isItemUrl + ? { + url: resolveUrl(inputUrl, metadataBase), + } + : { + ...item, + // Update image descriptor url + url: resolveUrl(inputUrl, metadataBase), + } +} + export function resolveImages( images: Twitter['images'], - metadataBase: ResolvedMetadata['metadataBase'] + metadataBase: ResolvedMetadataBase ): NonNullable['images'] export function resolveImages( images: OpenGraph['images'], - metadataBase: ResolvedMetadata['metadataBase'] + metadataBase: ResolvedMetadataBase ): NonNullable['images'] export function resolveImages( images: OpenGraph['images'] | Twitter['images'], - metadataBase: ResolvedMetadata['metadataBase'] + metadataBase: ResolvedMetadataBase ): | NonNullable['images'] | NonNullable['images'] { const resolvedImages = resolveAsArrayOrUndefined(images) if (!resolvedImages) return resolvedImages + const { isMetadataBaseMissing, fallbackMetadataBase } = + getSocialImageFallbackMetadataBase(metadataBase) const nonNullableImages = [] for (const item of resolvedImages) { - if (!item) continue - const isItemUrl = isStringOrURL(item) - const inputUrl = isItemUrl ? item : item.url - if (!inputUrl) continue - - nonNullableImages.push( - isItemUrl - ? { - url: resolveUrl(item, metadataBase), - } - : { - ...item, - // Update image descriptor url - url: resolveUrl(item.url, metadataBase), - } + const resolvedItem = resolveAndValidateImage( + item, + fallbackMetadataBase, + isMetadataBaseMissing ) + if (!resolvedItem) continue + + nonNullableImages.push(resolvedItem) } return nonNullableImages @@ -94,9 +116,26 @@ function getFieldsByOgType(ogType: OpenGraphType | undefined) { } } +function validateResolvedImageUrl( + inputUrl: string | URL, + fallbackMetadataBase: NonNullable, + isMetadataBaseMissing: boolean +): void { + // Only warn on the image url that needs to be resolved with metadataBase + if ( + typeof inputUrl === 'string' && + !isFullStringUrl(inputUrl) && + isMetadataBaseMissing + ) { + warnOnce( + `metadataBase property in metadata export is not set for resolving social open graph or twitter images, using "${fallbackMetadataBase.origin}". See https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase` + ) + } +} + export const resolveOpenGraph: FieldResolverExtraArgs< 'openGraph', - [ResolvedMetadata['metadataBase'], MetadataContext, string | null] + [ResolvedMetadataBase, MetadataContext, string | null] > = (openGraph, metadataBase, { pathname }, titleTemplate) => { if (!openGraph) return null @@ -114,9 +153,7 @@ export const resolveOpenGraph: FieldResolverExtraArgs< } } } - - const imageMetadataBase = getSocialImageFallbackMetadataBase(metadataBase) - target.images = resolveImages(og.images, imageMetadataBase) + target.images = resolveImages(og.images, metadataBase) } const resolved = { @@ -142,7 +179,7 @@ const TwitterBasicInfoKeys = [ export const resolveTwitter: FieldResolverExtraArgs< 'twitter', - [ResolvedMetadata['metadataBase'], string | null] + [ResolvedMetadataBase, string | null] > = (twitter, metadataBase, titleTemplate) => { if (!twitter) return null let card = 'card' in twitter ? twitter.card : undefined @@ -153,8 +190,8 @@ export const resolveTwitter: FieldResolverExtraArgs< for (const infoKey of TwitterBasicInfoKeys) { resolved[infoKey] = twitter[infoKey] || null } - const imageMetadataBase = getSocialImageFallbackMetadataBase(metadataBase) - resolved.images = resolveImages(twitter.images, imageMetadataBase) + + resolved.images = resolveImages(twitter.images, metadataBase) card = card || (resolved.images?.length ? 'summary_large_image' : 'summary') resolved.card = card diff --git a/packages/next/src/lib/metadata/resolvers/resolve-url.ts b/packages/next/src/lib/metadata/resolvers/resolve-url.ts index 0cc0cd3483f231..9550548b6020a4 100644 --- a/packages/next/src/lib/metadata/resolvers/resolve-url.ts +++ b/packages/next/src/lib/metadata/resolvers/resolve-url.ts @@ -1,5 +1,5 @@ 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 @@ -11,19 +11,20 @@ function createLocalMetadataBase() { // For deployment url for metadata routes, prefer to use the deployment url if possible // as these routes are unique to the deployments url. -export function getSocialImageFallbackMetadataBase( - metadataBase: URL | null -): URL | null { +export function getSocialImageFallbackMetadataBase(metadataBase: URL | null): { + fallbackMetadataBase: URL + isMetadataBaseMissing: boolean +} { const isMetadataBaseMissing = !metadataBase const defaultMetadataBase = createLocalMetadataBase() const deploymentUrl = process.env.VERCEL_URL && new URL(`https://${process.env.VERCEL_URL}`) - let fallbackMetadata + let fallbackMetadataBase if (process.env.NODE_ENV === 'development') { - fallbackMetadata = defaultMetadataBase + fallbackMetadataBase = defaultMetadataBase } else { - fallbackMetadata = + fallbackMetadataBase = process.env.NODE_ENV === 'production' && deploymentUrl && process.env.VERCEL_ENV === 'preview' @@ -31,14 +32,10 @@ export function getSocialImageFallbackMetadataBase( : metadataBase || deploymentUrl || defaultMetadataBase } - if (isMetadataBaseMissing) { - Log.warnOnce('') - Log.warnOnce( - `metadata.metadataBase is not set for resolving social open graph or twitter images, using "${fallbackMetadata.origin}". See https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase` - ) + return { + fallbackMetadataBase, + isMetadataBaseMissing, } - - return fallbackMetadata } function resolveUrl(url: null | undefined, metadataBase: URL | null): null diff --git a/packages/next/src/lib/url.ts b/packages/next/src/lib/url.ts new file mode 100644 index 00000000000000..32aea560824901 --- /dev/null +++ b/packages/next/src/lib/url.ts @@ -0,0 +1,13 @@ +const DUMMY_ORIGIN = 'http://n' + +function getUrlWithoutHost(url: string) { + return new URL(url, DUMMY_ORIGIN) +} + +export function getPathname(url: string) { + return getUrlWithoutHost(url).pathname +} + +export function isFullStringUrl(url: string) { + return /https?:\/\//.test(url) +} diff --git a/test/e2e/app-dir/metadata-missing-metadata-base/app/page.js b/test/e2e/app-dir/metadata-missing-metadata-base/app/page.js deleted file mode 100644 index eba737535cc7e9..00000000000000 --- a/test/e2e/app-dir/metadata-missing-metadata-base/app/page.js +++ /dev/null @@ -1,9 +0,0 @@ -import React from 'react' - -export default function Page() { - return <>hello index -} - -export const metadata = { - title: 'index page', -} diff --git a/test/e2e/app-dir/metadata-missing-metadata-base/next.config.js b/test/e2e/app-dir/metadata-missing-metadata-base/next.config.js deleted file mode 100644 index 4ba52ba2c8df67..00000000000000 --- a/test/e2e/app-dir/metadata-missing-metadata-base/next.config.js +++ /dev/null @@ -1 +0,0 @@ -module.exports = {} diff --git a/test/e2e/app-dir/metadata-warnings/app/absolute-url-og/page.js b/test/e2e/app-dir/metadata-warnings/app/absolute-url-og/page.js new file mode 100644 index 00000000000000..3e8b9222002b44 --- /dev/null +++ b/test/e2e/app-dir/metadata-warnings/app/absolute-url-og/page.js @@ -0,0 +1,9 @@ +export default function Page() { + return 'absolute url og page' +} + +export const metadata = { + openGraph: { + images: 'https://repository-images.githubusercontent.com/123', + }, +} diff --git a/test/e2e/app-dir/metadata-missing-metadata-base/app/layout.js b/test/e2e/app-dir/metadata-warnings/app/layout.js similarity index 85% rename from test/e2e/app-dir/metadata-missing-metadata-base/app/layout.js rename to test/e2e/app-dir/metadata-warnings/app/layout.js index 762515029332e8..750eb927b19801 100644 --- a/test/e2e/app-dir/metadata-missing-metadata-base/app/layout.js +++ b/test/e2e/app-dir/metadata-warnings/app/layout.js @@ -1,7 +1,6 @@ export default function Layout({ children }) { return ( - {children} ) diff --git a/test/e2e/app-dir/metadata-missing-metadata-base/app/opengraph-image.png b/test/e2e/app-dir/metadata-warnings/app/og-image-convention/opengraph-image.png similarity index 100% rename from test/e2e/app-dir/metadata-missing-metadata-base/app/opengraph-image.png rename to test/e2e/app-dir/metadata-warnings/app/og-image-convention/opengraph-image.png diff --git a/test/e2e/app-dir/metadata-warnings/app/og-image-convention/page.js b/test/e2e/app-dir/metadata-warnings/app/og-image-convention/page.js new file mode 100644 index 00000000000000..9c4a21008113f0 --- /dev/null +++ b/test/e2e/app-dir/metadata-warnings/app/og-image-convention/page.js @@ -0,0 +1,3 @@ +export default function Page() { + return 'page' +} diff --git a/test/e2e/app-dir/metadata-missing-metadata-base/app/unsupported-metadata/page.js b/test/e2e/app-dir/metadata-warnings/app/unsupported-metadata/page.js similarity index 100% rename from test/e2e/app-dir/metadata-missing-metadata-base/app/unsupported-metadata/page.js rename to test/e2e/app-dir/metadata-warnings/app/unsupported-metadata/page.js diff --git a/test/e2e/app-dir/metadata-missing-metadata-base/index.test.ts b/test/e2e/app-dir/metadata-warnings/index.test.ts similarity index 65% rename from test/e2e/app-dir/metadata-missing-metadata-base/index.test.ts rename to test/e2e/app-dir/metadata-warnings/index.test.ts index 5b9ae08d06c729..5fc0facc543d24 100644 --- a/test/e2e/app-dir/metadata-missing-metadata-base/index.test.ts +++ b/test/e2e/app-dir/metadata-warnings/index.test.ts @@ -1,29 +1,36 @@ import { createNextDescribe } from 'e2e-utils' import { fetchViaHTTP } from 'next-test-utils' +const METADATA_BASE_WARN_STRING = + 'metadataBase property in metadata export is not set for resolving social open graph or twitter images,' + createNextDescribe( 'app dir - metadata missing metadataBase', { files: __dirname, skipDeployment: true, }, - ({ next, isNextStart }) => { + ({ next, isNextDev }) => { // If it's start mode, we get the whole logs since they're from build process. // If it's dev mode, we get the logs after request function getCliOutput(logStartPosition: number) { - return isNextStart - ? next.cliOutput - : next.cliOutput.slice(logStartPosition) + return isNextDev ? next.cliOutput.slice(logStartPosition) : next.cliOutput + } + + if (isNextDev) { + it('should not warn metadataBase is missing if there is only absolute url', async () => { + const logStartPosition = next.cliOutput.length + await fetchViaHTTP(next.url, '/absolute-url-og') + const output = getCliOutput(logStartPosition) + expect(output).not.toInclude(METADATA_BASE_WARN_STRING) + }) } it('should fallback to localhost if metadataBase is missing for absolute urls resolving', async () => { const logStartPosition = next.cliOutput.length - await fetchViaHTTP(next.url, '/') - // + await fetchViaHTTP(next.url, '/og-image-convention') const output = getCliOutput(logStartPosition) - expect(output).toInclude( - 'metadata.metadataBase is not set for resolving social open graph or twitter images,' - ) + expect(output).toInclude(METADATA_BASE_WARN_STRING) expect(output).toMatch(/using "http:\/\/localhost:\d+/) expect(output).toInclude( '. See https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase' From 6f17077ce3c88580439beb642f3aafe0e11b3e6d Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Thu, 15 Feb 2024 18:57:15 +0100 Subject: [PATCH 2/3] Fix trailing slash for canonical url (#62109) 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. 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 --- packages/next/src/export/index.ts | 1 + packages/next/src/lib/metadata/metadata.tsx | 3 ++ .../src/lib/metadata/resolve-metadata.test.ts | 1 + .../lib/metadata/resolvers/resolve-basics.ts | 42 +++++++++++-------- .../metadata/resolvers/resolve-opengraph.ts | 10 +++-- .../src/lib/metadata/resolvers/resolve-url.ts | 17 ++++++-- .../next/src/lib/metadata/types/resolvers.ts | 1 + .../next/src/server/app-render/app-render.tsx | 3 ++ packages/next/src/server/app-render/types.ts | 1 + packages/next/src/server/base-server.ts | 2 + .../metadata-dynamic-routes/index.test.ts | 2 +- .../metadata-dynamic-routes/next.config.js | 1 + test/e2e/app-dir/metadata/metadata.test.ts | 2 +- test/e2e/app-dir/trailingslash/app/layout.js | 7 ++++ .../trailingslash/trailingslash.test.ts | 13 ++++++ 15 files changed, 80 insertions(+), 26 deletions(-) diff --git a/packages/next/src/export/index.ts b/packages/next/src/export/index.ts index 28d69212590671..177a73de6d60b2 100644 --- a/packages/next/src/export/index.ts +++ b/packages/next/src/export/index.ts @@ -474,6 +474,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 index 45fefdff81d875..925541b4b97383 100644 --- a/packages/next/src/lib/metadata/metadata.tsx +++ b/packages/next/src/lib/metadata/metadata.tsx @@ -39,6 +39,7 @@ export function createMetadataComponents({ tree, pathname, searchParams, + trailingSlash, getDynamicParamFromSegment, appUsingSizeAdjustment, errorType, @@ -46,12 +47,14 @@ export function createMetadataComponents({ tree: LoaderTree pathname: string searchParams: { [key: string]: any } + trailingSlash: boolean getDynamicParamFromSegment: GetDynamicParamFromSegment appUsingSizeAdjustment: boolean errorType?: 'not-found' | 'redirect' }): [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 index 9c6f0b526a2e94..06b42fe46ea091 100644 --- 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 index a3168f50eb30aa..bf1913520bda8f 100644 --- 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 { 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 index 9bb526e62d7a3f..31648e2ed70555 100644 --- a/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts +++ b/packages/next/src/lib/metadata/resolvers/resolve-opengraph.ts @@ -135,8 +135,8 @@ function validateResolvedImageUrl( export const resolveOpenGraph: FieldResolverExtraArgs< 'openGraph', - [ResolvedMetadataBase, MetadataContext, string | null] -> = (openGraph, metadataBase, { pathname }, titleTemplate) => { + [ResolvedMetadata['metadataBase'], MetadataContext, string | null] +> = (openGraph, metadataBase, metadataContext, titleTemplate) => { if (!openGraph) return null function resolveProps(target: ResolvedOpenGraph, og: OpenGraph) { @@ -163,7 +163,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 index 9550548b6020a4..8ddc6aa85c36c0 100644 --- a/packages/next/src/lib/metadata/resolvers/resolve-url.ts +++ b/packages/next/src/lib/metadata/resolvers/resolve-url.ts @@ -80,12 +80,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 index fddabb3b75e0ab..fcc9f2b41786ee 100644 --- 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 index ceec35c11f8aa6..591337cd9c2695 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -265,6 +265,7 @@ async function generateFlight( tree: loaderTree, pathname: urlPathname, searchParams: providedSearchParams, + trailingSlash: ctx.renderOpts.trailingSlash, getDynamicParamFromSegment, appUsingSizeAdjustment, }) @@ -391,6 +392,7 @@ async function ReactServerApp({ errorType: asNotFound ? 'not-found' : undefined, pathname: urlPathname, searchParams: providedSearchParams, + trailingSlash: ctx.renderOpts.trailingSlash, getDynamicParamFromSegment: getDynamicParamFromSegment, appUsingSizeAdjustment: appUsingSizeAdjustment, }) @@ -467,6 +469,7 @@ async function ReactServerError({ const [MetadataTree] = createMetadataComponents({ tree, pathname: urlPathname, + trailingSlash: ctx.renderOpts.trailingSlash, errorType, searchParams: providedSearchParams, getDynamicParamFromSegment, diff --git a/packages/next/src/server/app-render/types.ts b/packages/next/src/server/app-render/types.ts index 800e7a53d5a243..d455e91b511423 100644 --- 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 index 6d88305c19e887..fdead159a34506 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -219,6 +219,7 @@ type BaseRenderOpts = { poweredByHeader: boolean buildId: string generateEtags: boolean + trailingSlash: boolean runtimeConfig?: { [key: string]: any } assetPrefix?: string canonicalBase: string @@ -506,6 +507,7 @@ export default abstract class Server { } this.renderOpts = { + 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 index 93df48a160c87e..f8526a3acd86c7 100644 --- a/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts +++ b/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts @@ -323,7 +323,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 index dc0c3a93b03ec2..36ba3614c43d11 100644 --- 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 index d489e4f0833bc8..a00d54aa7cbc9f 100644 --- 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 index 05b841b280b3fc..179bbf228e599d 100644 --- 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 }) { ) } + +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 index 3b2f91b7ba72a1..2d71308630f4e2 100644 --- 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') From abf36d7cb605e539ab4f2403981a7b819dba932a Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 28 Feb 2024 00:55:27 +0100 Subject: [PATCH 3/3] Fix metadata json manifest convention (#62615) ### What Change from processing the file with `next-metatdata-route-loader` directly into passing the file as loader query, and leave an empty resource file for it. This will resolve the error that users were seeing with `manifest.json` convention. ``` Import trace for requested module: ../../../../packages/next/dist/build/webpack/loaders/next-metadata-route-loader.js?page=%2Fmanifest.jso n%2Froute&isDynamic=0!./app/manifest.json?__next_metadata_route__ getStaticAssetRouteCode page /manifest.json/route this.resourcePath /Users/huozhi/workspace/next.js/tes t/e2e/app-dir/metadata-json-manifest/app/manifest.json ``` ### Why I looked at the loader process that the final resource processed by webpack is `json!next-metadata-route-loader...`, which means the builtin json loader processing json file after the metadata route loader. I didn't get chance to solve the ordering issue, so I changed the resourcePath to empty "", and pass the file path as query into the loader to avoid json-loader processing it after transpilation. Fixes #59923 Closes NEXT-2630 Closes NEXT-2439 --- .../build/webpack/loaders/next-app-loader.ts | 3 ++- .../loaders/next-metadata-route-loader.ts | 21 ++++++++++-------- .../metadata-json-manifest/app/layout.js | 12 ++++++++++ .../metadata-json-manifest/app/manifest.json | 6 +++++ .../metadata-json-manifest/app/page.js | 3 +++ .../metadata-json-manifest/index.test.ts | 22 +++++++++++++++++++ 6 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 test/e2e/app-dir/metadata-json-manifest/app/layout.js create mode 100644 test/e2e/app-dir/metadata-json-manifest/app/manifest.json create mode 100644 test/e2e/app-dir/metadata-json-manifest/app/page.js create mode 100644 test/e2e/app-dir/metadata-json-manifest/index.test.ts diff --git a/packages/next/src/build/webpack/loaders/next-app-loader.ts b/packages/next/src/build/webpack/loaders/next-app-loader.ts index 5fdae7e86751f8..388480d3c760b8 100644 --- a/packages/next/src/build/webpack/loaders/next-app-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-app-loader.ts @@ -124,8 +124,9 @@ async function createAppRouteCode({ resolvedPagePath = `next-metadata-route-loader?${stringify({ page, + filePath: resolvedPagePath, isDynamic: isDynamic ? '1' : '0', - })}!${resolvedPagePath}${`?${WEBPACK_RESOURCE_QUERIES.metadataRoute}`}` + })}!?${WEBPACK_RESOURCE_QUERIES.metadataRoute}` } const pathname = new AppPathnameNormalizer().normalize(page) diff --git a/packages/next/src/build/webpack/loaders/next-metadata-route-loader.ts b/packages/next/src/build/webpack/loaders/next-metadata-route-loader.ts index 49aa5782c8d319..7fb4f62792fe58 100644 --- a/packages/next/src/build/webpack/loaders/next-metadata-route-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-metadata-route-loader.ts @@ -22,6 +22,7 @@ const cacheHeader = { type MetadataRouteLoaderOptions = { page: string + filePath: string isDynamic: '1' | '0' } @@ -46,7 +47,6 @@ function getContentType(resourcePath: string) { return 'text/plain' } -// Strip metadata resource query string from `import.meta.url` to make sure the fs.readFileSync get the right path. async function getStaticAssetRouteCode( resourcePath: string, fileBaseName: string @@ -58,6 +58,7 @@ async function getStaticAssetRouteCode( ? cacheHeader.none : cacheHeader.longCache const code = `\ +/* static asset route */ import { NextResponse } from 'next/server' const contentType = ${JSON.stringify(getContentType(resourcePath))} @@ -82,6 +83,7 @@ export const dynamic = 'force-static' function getDynamicTextRouteCode(resourcePath: string) { return `\ +/* dynamic asset route */ import { NextResponse } from 'next/server' import handler from ${JSON.stringify(resourcePath)} import { resolveRouteData } from 'next/dist/build/webpack/loaders/metadata/resolve-route-data' @@ -108,6 +110,7 @@ export async function GET() { // /[id]/route.js function getDynamicImageRouteCode(resourcePath: string) { return `\ +/* dynamic image route */ import { NextResponse } from 'next/server' import * as userland from ${JSON.stringify(resourcePath)} @@ -159,6 +162,7 @@ async function getDynamicSiteMapRouteCode( page.includes('[__metadata_id__]') ) { staticGenerationCode = `\ +/* dynamic sitemap route */ export async function generateStaticParams() { const sitemaps = generateSitemaps ? await generateSitemaps() : [] const params = [] @@ -224,26 +228,25 @@ ${staticGenerationCode} ` return code } -// `import.meta.url` is the resource name of the current module. + // When it's static route, it could be favicon.ico, sitemap.xml, robots.txt etc. // TODO-METADATA: improve the cache control strategy const nextMetadataRouterLoader: webpack.LoaderDefinitionFunction = async function () { - const { resourcePath } = this - const { page, isDynamic } = this.getOptions() - const { name: fileBaseName } = getFilenameAndExtension(resourcePath) + const { page, isDynamic, filePath } = this.getOptions() + const { name: fileBaseName } = getFilenameAndExtension(filePath) let code = '' if (isDynamic === '1') { if (fileBaseName === 'robots' || fileBaseName === 'manifest') { - code = getDynamicTextRouteCode(resourcePath) + code = getDynamicTextRouteCode(filePath) } else if (fileBaseName === 'sitemap') { - code = await getDynamicSiteMapRouteCode(resourcePath, page, this) + code = await getDynamicSiteMapRouteCode(filePath, page, this) } else { - code = getDynamicImageRouteCode(resourcePath) + code = getDynamicImageRouteCode(filePath) } } else { - code = await getStaticAssetRouteCode(resourcePath, fileBaseName) + code = await getStaticAssetRouteCode(filePath, fileBaseName) } return code diff --git a/test/e2e/app-dir/metadata-json-manifest/app/layout.js b/test/e2e/app-dir/metadata-json-manifest/app/layout.js new file mode 100644 index 00000000000000..8525f5f8c0b2a1 --- /dev/null +++ b/test/e2e/app-dir/metadata-json-manifest/app/layout.js @@ -0,0 +1,12 @@ +export const metadata = { + title: 'Next.js', + description: 'Generated by Next.js', +} + +export default function RootLayout({ children }) { + return ( + + {children} + + ) +} diff --git a/test/e2e/app-dir/metadata-json-manifest/app/manifest.json b/test/e2e/app-dir/metadata-json-manifest/app/manifest.json new file mode 100644 index 00000000000000..4e09c9827989b6 --- /dev/null +++ b/test/e2e/app-dir/metadata-json-manifest/app/manifest.json @@ -0,0 +1,6 @@ +{ + "name": "My Next.js Application", + "short_name": "Next.js App", + "description": "An application built with Next.js", + "start_url": "/" +} diff --git a/test/e2e/app-dir/metadata-json-manifest/app/page.js b/test/e2e/app-dir/metadata-json-manifest/app/page.js new file mode 100644 index 00000000000000..cbce1149e82574 --- /dev/null +++ b/test/e2e/app-dir/metadata-json-manifest/app/page.js @@ -0,0 +1,3 @@ +export default function page() { + return 'page.js' +} diff --git a/test/e2e/app-dir/metadata-json-manifest/index.test.ts b/test/e2e/app-dir/metadata-json-manifest/index.test.ts new file mode 100644 index 00000000000000..9f58610e7346fa --- /dev/null +++ b/test/e2e/app-dir/metadata-json-manifest/index.test.ts @@ -0,0 +1,22 @@ +import { createNextDescribe } from 'e2e-utils' + +createNextDescribe( + 'app-dir metadata-json-manifest', + { + files: __dirname, + skipDeployment: true, + }, + ({ next }) => { + it('should support metadata.json manifest', async () => { + const response = await next.fetch('/manifest.json') + expect(response.status).toBe(200) + const json = await response.json() + expect(json).toEqual({ + name: 'My Next.js Application', + short_name: 'Next.js App', + description: 'An application built with Next.js', + start_url: '/', + }) + }) + } +)