From c0751e8c81aa6c4ae2bc089f1396e6cecef68e61 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Thu, 6 Apr 2023 00:40:17 +0200 Subject: [PATCH] Create unique route path for og under group routes (#47985) ### What When using dynamic metadata image rouets (such as `opengraph-image.js`) under group routes, the generated urls were still normalized. In this case it might have conflicts with those ones not under group routes. For instance `app/(post)/opengraph-image.js` could have same url with `/app/opengraph-image.js`. In reality we want them to be different route, unlike layout or pages. ### How So when we found `()` or `@` signs from the metadata image urls, we'll generate a unqiue suffix (`-\d{6}`) and append to the generated url. So they can be isolated from the ones are not under special convention routes. Closes NEXT-937 --- .../webpack/loaders/metadata/discover.ts | 8 ++-- .../build/webpack/loaders/next-app-loader.ts | 5 +-- .../loaders/next-metadata-image-loader.ts | 22 +++++++---- .../src/lib/metadata/get-metadata-route.ts | 27 +++++++++++-- .../next/src/lib/metadata/resolve-metadata.ts | 5 +-- .../next/src/server/app-render/app-render.tsx | 4 +- packages/next/src/shared/lib/hash.ts | 9 +++++ .../app/(group)/blog/page.tsx | 3 ++ .../app/(group)/layout.tsx | 14 +++++++ .../app/(group)/opengraph-image.tsx | 22 +++++++++++ .../metadata-dynamic-routes/index.test.ts | 38 +++++++++++++++---- 11 files changed, 128 insertions(+), 29 deletions(-) create mode 100644 packages/next/src/shared/lib/hash.ts create mode 100644 test/e2e/app-dir/metadata-dynamic-routes/app/(group)/blog/page.tsx create mode 100644 test/e2e/app-dir/metadata-dynamic-routes/app/(group)/layout.tsx create mode 100644 test/e2e/app-dir/metadata-dynamic-routes/app/(group)/opengraph-image.tsx diff --git a/packages/next/src/build/webpack/loaders/metadata/discover.ts b/packages/next/src/build/webpack/loaders/metadata/discover.ts index 00236c4b86..34e5779513 100644 --- a/packages/next/src/build/webpack/loaders/metadata/discover.ts +++ b/packages/next/src/build/webpack/loaders/metadata/discover.ts @@ -6,6 +6,7 @@ import type { import path from 'path' import { stringify } from 'querystring' import { STATIC_METADATA_IMAGES } from '../../../../lib/metadata/is-metadata-route' +import { normalizeAppPath } from '../../../../shared/lib/router/utils/app-paths' const METADATA_TYPE = 'metadata' @@ -54,13 +55,13 @@ async function enumMetadataFiles( export async function createStaticMetadataFromRoute( resolvedDir: string, { - route, + segment, resolvePath, isRootLayer, loaderContext, pageExtensions, }: { - route: string + segment: string resolvePath: (pathname: string) => Promise isRootLayer: boolean loaderContext: webpack.LoaderContext @@ -98,7 +99,8 @@ export async function createStaticMetadataFromRoute( const imageModuleImportSource = `next-metadata-image-loader?${stringify( { type, - route, + segment, + route: normalizeAppPath(segment), pageExtensions, } )}!${filepath}${METADATA_RESOURCE_QUERY}` 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 6ea8379685..9ccb5c85a3 100644 --- a/packages/next/src/build/webpack/loaders/next-app-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-app-loader.ts @@ -20,7 +20,6 @@ import { isAppRouteRoute } from '../../../lib/is-app-route-route' import { isMetadataRoute } from '../../../lib/metadata/is-metadata-route' import { NextConfig } from '../../../server/config-shared' import { AppPathnameNormalizer } from '../../../server/future/normalizers/built/app/app-pathname-normalizer' -import { normalizeAppPath } from '../../../shared/lib/router/utils/app-paths' export type AppLoaderOptions = { name: string @@ -255,7 +254,7 @@ async function createTreeCodeFromPath( if (resolvedRouteDir) { metadata = await createStaticMetadataFromRoute(resolvedRouteDir, { - route: normalizeAppPath(segmentPath), + segment: segmentPath, resolvePath, isRootLayer, loaderContext, @@ -348,7 +347,7 @@ async function createTreeCodeFromPath( )}), ${JSON.stringify(filePath)}],` }) .join('\n')} - ${definedFilePaths.length ? createMetadataExportsCode(metadata) : ''} + ${createMetadataExportsCode(metadata)} } ]` } diff --git a/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts b/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts index ea6664142c..bb0c072a0c 100644 --- a/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts @@ -13,13 +13,14 @@ import { imageExtMimeTypeMap } from '../../../lib/mime-type' interface Options { route: string + segment: string type: PossibleImageFileNameConvention pageExtensions: string[] } async function nextMetadataImageLoader(this: any, content: Buffer) { const options: Options = this.getOptions() - const { type, route, pageExtensions } = options + const { type, route, segment, pageExtensions } = options const numericSizes = type === 'twitter' || type === 'openGraph' const { resourcePath, rootContext: context } = this const { name: fileNameBase, ext } = path.parse(resourcePath) @@ -44,9 +45,8 @@ async function nextMetadataImageLoader(this: any, content: Buffer) { ) const isDynamicResource = pageExtensions.includes(extension) - const pageRoute = - (isDynamicResource ? fileNameBase : interpolatedName) + - (contentHash ? '?' + contentHash : '') + const pageRoute = isDynamicResource ? fileNameBase : interpolatedName + const hashQuery = contentHash ? '?' + contentHash : '' if (isDynamicResource) { // re-export and spread as `exportedImageData` to avoid non-exported error @@ -55,17 +55,23 @@ async function nextMetadataImageLoader(this: any, content: Buffer) { import * as exported from ${JSON.stringify(resourcePath)} import { interpolateDynamicPath } from 'next/dist/server/server-utils' import { getNamedRouteRegex } from 'next/dist/shared/lib/router/utils/route-regex' + import { getMetadataRouteSuffix } from 'next/dist/lib/metadata/get-metadata-route' const exportedImageData = { ...exported } export default (props) => { const pathname = ${JSON.stringify(route)} const routeRegex = getNamedRouteRegex(pathname, false) + const segment = ${JSON.stringify(segment)} const route = interpolateDynamicPath(pathname, props.params, routeRegex) + const suffix = getMetadataRouteSuffix(segment) + const routeSuffix = suffix ? \`-\${suffix}\` : '' const imageData = { alt: exportedImageData.alt, - type: exportedImageData.contentType, - url: path.join(route, ${JSON.stringify(pageRoute)}), + type: exportedImageData.contentType || 'image/png', + url: path.join(route, ${JSON.stringify( + pageRoute + )} + routeSuffix + ${JSON.stringify(hashQuery)}), } const { size } = exportedImageData if (size) { @@ -118,7 +124,9 @@ async function nextMetadataImageLoader(this: any, content: Buffer) { return { ...imageData, - url: path.join(route, ${JSON.stringify(pageRoute)}), + url: path.join(route, ${JSON.stringify(pageRoute)} + ${JSON.stringify( + hashQuery + )}), } }` } diff --git a/packages/next/src/lib/metadata/get-metadata-route.ts b/packages/next/src/lib/metadata/get-metadata-route.ts index 325b456977..b5b803555d 100644 --- a/packages/next/src/lib/metadata/get-metadata-route.ts +++ b/packages/next/src/lib/metadata/get-metadata-route.ts @@ -1,4 +1,23 @@ import { isMetadataRoute } from './is-metadata-route' +import path from '../../shared/lib/isomorphic/path' +import { djb2Hash } from '../../shared/lib/hash' + +/* + * If there's special convention like (...) or @ in the page path, + * Give it a unique hash suffix to avoid conflicts + * + * e.g. + * /app/open-graph.tsx -> /open-graph/route + * /app/(post)/open-graph.tsx -> /open-graph/route-123456 + */ +export function getMetadataRouteSuffix(page: string) { + let suffix = '' + + if ((page.includes('(') && page.includes(')')) || page.includes('@')) { + suffix = djb2Hash(page).toString().slice(0, 6) + } + return suffix +} /** * Map metadata page key to the corresponding route @@ -12,8 +31,10 @@ import { isMetadataRoute } from './is-metadata-route' export function normalizeMetadataRoute(page: string) { let route = page if (isMetadataRoute(page)) { - // TODO-METADATA: add dynamic routes for metadata images. - // Better to move the extension appending to early phase. + // Remove the file extension, e.g. /route-path/robots.txt -> /route-path + const pathnamePrefix = page.slice(0, -(path.basename(page).length + 1)) + const suffix = getMetadataRouteSuffix(pathnamePrefix) + if (route === '/sitemap') { route += '.xml' } @@ -26,7 +47,7 @@ export function normalizeMetadataRoute(page: string) { // Support both / and custom routes //route.ts. // If it's a metadata file route, we need to append /route to the page. if (!route.endsWith('/route')) { - route = `${route}/route` + route = `${route}${suffix ? `-${suffix}` : ''}/route` } } return route diff --git a/packages/next/src/lib/metadata/resolve-metadata.ts b/packages/next/src/lib/metadata/resolve-metadata.ts index 3f89d9bb12..398483c4f3 100644 --- a/packages/next/src/lib/metadata/resolve-metadata.ts +++ b/packages/next/src/lib/metadata/resolve-metadata.ts @@ -257,13 +257,13 @@ async function resolveStaticMetadata(components: ComponentsType, props: any) { // [layout.metadata, static files metadata] -> ... -> [page.metadata, static files metadata] export async function collectMetadata({ loaderTree, + metadataItems: array, props, - array, route, }: { loaderTree: LoaderTree + metadataItems: MetadataItems props: any - array: MetadataItems route: string }) { const [mod, modType] = await getLayoutOrPageModule(loaderTree) @@ -289,7 +289,6 @@ export async function accumulateMetadata( options: MetadataAccumulationOptions ): Promise { const resolvedMetadata = createDefaultMetadata() - const resolvers: ((value: ResolvedMetadata) => void)[] = [] const generateMetadataResults: (Metadata | Promise)[] = [] diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 56882423bb..23374483e4 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -384,8 +384,8 @@ export async function renderToHTMLOrFlight( await collectMetadata({ loaderTree: tree, + metadataItems, props: layerProps, - array: metadataItems, route: currentTreePrefix // __PAGE__ shouldn't be shown in a route .filter((s) => s !== PAGE_SEGMENT_KEY) @@ -396,8 +396,8 @@ export async function renderToHTMLOrFlight( const childTree = parallelRoutes[key] await resolveMetadata({ tree: childTree, - parentParams: currentParams, metadataItems, + parentParams: currentParams, treePrefix: currentTreePrefix, }) } diff --git a/packages/next/src/shared/lib/hash.ts b/packages/next/src/shared/lib/hash.ts new file mode 100644 index 0000000000..aaaffdd501 --- /dev/null +++ b/packages/next/src/shared/lib/hash.ts @@ -0,0 +1,9 @@ +// http://www.cse.yorku.ca/~oz/hash.html +export function djb2Hash(str: string) { + let hash = 5381 + for (let i = 0; i < str.length; i++) { + const char = str.charCodeAt(i) + hash = (hash << 5) + hash + char + } + return Math.abs(hash) +} diff --git a/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/blog/page.tsx b/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/blog/page.tsx new file mode 100644 index 0000000000..bfc1f40e6e --- /dev/null +++ b/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/blog/page.tsx @@ -0,0 +1,3 @@ +export default function page() { + return <>{`(group)blog`} +} diff --git a/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/layout.tsx b/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/layout.tsx new file mode 100644 index 0000000000..7499f2920e --- /dev/null +++ b/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/layout.tsx @@ -0,0 +1,14 @@ +export default function layout({ children }) { + return ( + + + +
{children}
+ + + ) +} + +export const metadata = { + title: 'Group Title', +} diff --git a/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/opengraph-image.tsx b/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/opengraph-image.tsx new file mode 100644 index 0000000000..2fe2403f5e --- /dev/null +++ b/test/e2e/app-dir/metadata-dynamic-routes/app/(group)/opengraph-image.tsx @@ -0,0 +1,22 @@ +import { ImageResponse } from 'next/server' + +export default function og() { + return new ImageResponse( + ( +
+ group route og +
+ ) + ) +} 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 f075fe4c05..56834ae7c7 100644 --- a/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts +++ b/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts @@ -119,11 +119,17 @@ createNextDescribe( }) it('should support params as argument in dynamic routes', async () => { - const bufferBig = await ( - await next.fetch('/dynamic/big/opengraph-image') - ).buffer() + const big$ = await next.render$('/dynamic/big') + const small$ = await next.render$('/dynamic/small') + const bigOgUrl = new URL( + big$('meta[property="og:image"]').attr('content') + ) + const smallOgUrl = new URL( + small$('meta[property="og:image"]').attr('content') + ) + const bufferBig = await (await next.fetch(bigOgUrl.pathname)).buffer() const bufferSmall = await ( - await next.fetch('/dynamic/small/opengraph-image') + await next.fetch(smallOgUrl.pathname) ).buffer() const sizeBig = imageSize(bufferBig) @@ -153,6 +159,18 @@ createNextDescribe( }) }) + it('should generate unique path for image routes under group routes', async () => { + const $ = await next.render$('/blog') + const ogImageUrl = $('meta[property="og:image"]').attr('content') + const ogImageUrlInstance = new URL(ogImageUrl) + const res = await next.fetch(ogImageUrlInstance.pathname) + + // generate unique path with suffix for image routes under group routes + expect(ogImageUrl).toMatch(/opengraph-image-\d{6}\?/) + expect(ogImageUrl).toMatch(hashRegex) + expect(res.status).toBe(200) + }) + it('should inject dynamic metadata properly to head', async () => { const $ = await next.render$('/') const $icon = $('link[rel="icon"]') @@ -182,14 +200,18 @@ createNextDescribe( if (isNextDeploy) { // absolute urls - expect(ogImageUrl).toMatch(/https:\/\/\w+.vercel.app\/opengraph-image/) + expect(ogImageUrl).toMatch( + /https:\/\/\w+.vercel.app\/opengraph-image\?/ + ) expect(twitterImageUrl).toMatch( - /https:\/\/\w+.vercel.app\/twitter-image/ + /https:\/\/\w+.vercel.app\/twitter-image\?/ ) } else { // absolute urls - expect(ogImageUrl).toMatch(/http:\/\/localhost:\d+\/opengraph-image/) - expect(twitterImageUrl).toMatch(/http:\/\/localhost:\d+\/twitter-image/) + expect(ogImageUrl).toMatch(/http:\/\/localhost:\d+\/opengraph-image\?/) + expect(twitterImageUrl).toMatch( + /http:\/\/localhost:\d+\/twitter-image\?/ + ) } expect(ogImageUrl).toMatch(hashRegex) expect(twitterImageUrl).toMatch(hashRegex)